Skip to content
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

update replace' to reflect the existence of optional capturing groups #126

Merged
merged 2 commits into from
Jan 6, 2021

Conversation

davidchambers
Copy link
Contributor

This pull request does for replace' what #32 did for match.

This pull request also fixes a few typos, renames a parameter (s2s), and removes the mutation of arguments in _replaceBy (formerly replaceBy).

I would like to propose another breaking change to this function, so if you merge this pull request please do not release a new version of the package immediately. :)

Comment on lines +101 to +109
foreign import _replaceBy
:: (forall r. r -> Maybe r)
-> (forall r. Maybe r)
-> Regex
-> (String -> Array (Maybe String) -> String)
-> String
-> String
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this import above the line that references it, for consistency with the placement of _match and _search.

Comment on lines +78 to +81
while (typeof (group = arguments[i++]) !== "number") {
groups.push(group == null ? nothing : just(group));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typeof check looks odd, but is necessary to handle both possible cases:

  • match, group1, group2, ..., offset, string (r contains no named capturing groups); and
  • match, group1, group2, ..., offset, string, groups (r contains named capturing groups).

Copy link
Member

@kl0tl kl0tl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! We’re going to make a major release of this library soon so if you have another breaking change planned feel free to open a pull request 🙂


foreign import replaceBy :: Regex -> (String -> Array String -> String) -> String -> String
-- | Transforms occurrences of the `Regex` using a function of the matched
-- | substring and a list of groups of type `Maybe String`, where `Nothing`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps ”captured substrings”, ”submatches” or ”submatch strings”, like in the previous comment? I’m not sure ”groups” makes much sense to denote what was captured.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I will make this change.

@JordanMartinez
Copy link
Contributor

Is there anything stopping this from getting merged? We have 2 approvals already.

@thomashoneyman
Copy link
Member

I believe we're giving @davidchambers a chance to update the documentation as per:
#126 (comment)

@JordanMartinez
Copy link
Contributor

Just curious, but if the change is that small, why not just make the edit ourselves so this can be merged? I'm not sure whether that would be impolite or not.

@thomashoneyman
Copy link
Member

I think in this case since the author indicated they plan to make the change that we should let them go through with it. If we reach the point where we need to release this library then we can make the edit and merge, but in general it is polite to allow pull request authors to make changes to their own pull requests whenever possible.

@davidchambers davidchambers force-pushed the replace branch 2 times, most recently from 7e9f362 to abaedd6 Compare January 6, 2021 08:25
@davidchambers
Copy link
Contributor Author

I have updated the wording as discussed.

Could someone investigate the build failure? It does not seem related to my changes.

@JordanMartinez
Copy link
Contributor

It's because CI is still on v0.14.0-rc3. We changed things in Newtype but it required some changes in the compiler as well. I've updated it to v0.14.0-rc5. CI passes now.

Anything else we need to do here? Or can this be merged?

@davidchambers
Copy link
Contributor Author

Thanks for the fix, @JordanMartinez. I transposed the commits; both are now green. As far as I am concerned, this pull request is ready to merge. :)

Once this pull request has been merged I will propose a second breaking change. ;)

@JordanMartinez JordanMartinez merged commit 5815ea9 into purescript:master Jan 6, 2021
@JordanMartinez
Copy link
Contributor

Merged!

@davidchambers davidchambers deleted the replace branch January 6, 2021 15:53
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.

4 participants