Skip to content

Newtypes for replace #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cryogenian opened this issue Jun 10, 2016 · 8 comments · Fixed by #66
Closed

Newtypes for replace #62

cryogenian opened this issue Jun 10, 2016 · 8 comments · Fixed by #66

Comments

@cryogenian
Copy link

The errors with replace where I mix up arguments occur surprisingly often.
What do you think, is it worth to add newtype wrappers for replacer/replacee part?

And in general something like Hay and Needle, SplitBy and other stuff?

@paf31
Copy link
Contributor

paf31 commented Jun 10, 2016

Since these strings aren't typically going to be placed in data structures, maybe type is enough to make the documentation clear?

@garyb
Copy link
Member

garyb commented Jun 10, 2016

Isn't that the suggestion though? Requiring newtype wrapping to ensure the strings must be specified in the right order: replace (Needle x) y (Hay z).

@paf31
Copy link
Contributor

paf31 commented Jun 10, 2016

I generally like newtype for this sort of thing, I just think it might be overkill in this case. A type synonym would show up in documentation and editor hints, which would be enough for me. This seems a bit different from something like EmailAddress vs PhoneNumber.

@cryogenian
Copy link
Author

Yeah, but this isn't for documentation purposes, but to remove weird errors and make declarations a bit clearer. And even with assumption that everyone uses right tooling (that's not true of course) editor hints are just recommendations not enfocements.

Other approach is something like

find :: {hay :: String, needle :: String} -> Maybe String  

It doesn't introduce new types, but currying is lost.

@cryogenian
Copy link
Author

Well except records 😄

@sharkdp
Copy link
Contributor

sharkdp commented Jun 10, 2016

type aliases would definitely be an improvement over replace :: String -> String -> String -> String.

However, I have to say I like the newtype approach (I'm not particularly keen about Needle and Hay). I think the overhead is managable, because typically you only need to wrap (and there is no unwrapping involved):

newtype Pattern = Pattern String
newtype Replacement = Replacement String

replace :: Pattern -> Replacement -> String -> String

replace (Pattern "http") (Replacement "https") "http://example.com"

@garyb
Copy link
Member

garyb commented Jun 10, 2016

If we were to newtype, I prefer @sharkdp's suggestion too - newtyping the first arguments and leaving the last one as a string.

@paf31
Copy link
Contributor

paf31 commented Jun 10, 2016

Agreed, that makes composition nicer too.

The other question is whether this is worth a breaking change again, when we haven't finished the 1.0 update cycle yet. (That's part of the reason I suggested using type synonyms to start off with).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants