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

Make RegexFlags a newtype and add a Newtype instance #159

Merged

Conversation

triallax
Copy link
Contributor

@triallax triallax commented Apr 15, 2022

Description of the change

Make RegexFlags a newtype and add a Newtype instance for it. I just thought that since it's merely wrapping a record, making it a newtype sounds appropriate.

I also refactored the Eq instance.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@garyb
Copy link
Member

garyb commented Apr 15, 2022

Maybe should be listed under breaking changes? I don't know whether we count breaking FFI representation as a breaking change, as I'm pretty sure we encourage people to generally rely on a particularly representation for a type (unless it's known to be a newtype, ironically).

@JordanMartinez
Copy link
Contributor

Maybe should be listed under breaking changes? I don't know whether we count breaking FFI representation as a breaking change, as I'm pretty sure we encourage people to generally rely on a particularly representation for a type (unless it's known to be a newtype, ironically).

As far as I know, we don't. And if they do, then they implicitly agreed to deal with such problems if/when they ever arise, right?

@MonoidMusician
Copy link
Contributor

How about adding a Newtype instance for it too?

@triallax
Copy link
Contributor Author

@MonoidMusician I'm fine with that, but does it bring any real benefits?

@MonoidMusician
Copy link
Contributor

Having a newtype instance does make some manipulations easier. But I'm asking for it mostly for consistency with the other newtypes in this library (e.g. Pattern and Replacement) and because it should be there if it can exist so that it doesn't have to be added in a follow-up PR once someone does need/want it down the road.

@triallax triallax force-pushed the make-regex-flags-a-newtype branch 2 times, most recently from 7b97360 to 2e0a912 Compare April 17, 2022 11:18
@triallax triallax changed the title Make RegexFlags a newtype Make RegexFlags a newtype and add a Newtype instance Apr 17, 2022
@thomashoneyman thomashoneyman merged commit b256597 into purescript:master Apr 17, 2022
@triallax triallax deleted the make-regex-flags-a-newtype branch April 17, 2022 14:26
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.

5 participants