Skip to content

simplify type of replace' #139

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
wants to merge 1 commit into from

Conversation

davidchambers
Copy link
Contributor

Commit message:

The match argument provided by String#replace (when given a function) is often irrelevant. If one does require access to the matched substring as a whole, one can parenthesize the whole pattern to capture it.

This is a breaking change, but I believe it would make replace' slightly more user-friendly.

This proposal was conceived while considering sanctuary-js/sanctuary#685 (as was #126).

Verified

This commit was signed with the committer’s verified signature.
davidchambers David Chambers
The ‘match’ argument provided by String#replace (when given a function)
is often irrelevant. If one does require access to the matched substring
as a whole, one can parenthesize the whole pattern to capture it.
@JordanMartinez
Copy link
Contributor

@kl0tl Thoughts on this one? You reviewed David's prior PR.

@MonoidMusician
Copy link
Contributor

I understand your point that "one can parenthesize the whole pattern to capture [the matched substring]", but I think in cases where the user really does need access to the whole match, having to deal with the partiality of getting it from the array of matches might be annoying, so it's not quite a free change. Plus there might be cases where you actually don't have access to the regex, if it's coming from a library or something.

@davidchambers
Copy link
Contributor Author

Very good points, @MonoidMusician.

in cases where the user really does need access to the whole match, having to deal with the partiality of getting it from the array of matches might be annoying

True. There is value in treating the whole match specially for this reason.

This highlights the impreciseness of the type we are using to represent captured substrings. Consider the regular expression ^(http(s)?)://. Array (Maybe String) is misleading for two reasons: the number of capturing groups is fixed, and the first capturing group is not optional. Ideally the type would be (String, Maybe String) in this case, but this would require parsing regular expressions to determine the number of capturing groups and their optionality.

I am not suggesting that we change the type of the captured substrings; Array (Maybe String) is a pragmatic approximation. It is unfortunate that extracting substrings from required capturing groups requires the use of a partial function (fromJust) or a dummy default value (fromMaybe 'XXX').

Plus there might be cases where you actually don't have access to the regex, if it's coming from a library or something.

I considered this in sanctuary-js/sanctuary#685, but the circumstances differ: JavaScript libraries can focus on common use cases because the underlying APIs are always available. What is right for Sanctuary may not be right for PureScript.

@kl0tl
Copy link
Member

kl0tl commented Jan 30, 2021

I don’t have a strong opinion on this but I don’t think that ignoring the first argument of the replacer function is annoying enough to warrant a breaking change. This would be perhaps more appropriate in an opinionated regex library.

@davidchambers
Copy link
Contributor Author

Thank you for considering this change. :)

@davidchambers davidchambers deleted the replace branch May 16, 2021 12:23
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 this pull request may close these issues.

None yet

4 participants