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

renamed getOrModify to matching #234

Merged
merged 1 commit into from
Aug 21, 2015
Merged

renamed getOrModify to matching #234

merged 1 commit into from
Aug 21, 2015

Conversation

julien-truffaut
Copy link
Member

I never liked the name getOrModify but I couldn't find the equivalent in haskell lens.

I completely missed it: https://hackage.haskell.org/package/lens-4.12.3/docs/src/Control-Lens-Prism.html#matching

julien-truffaut added a commit that referenced this pull request Aug 21, 2015
renamed getOrModify to matching
@julien-truffaut julien-truffaut merged commit 5c8de8e into master Aug 21, 2015
@julien-truffaut julien-truffaut deleted the matching branch August 21, 2015 19:11
@NightRa
Copy link
Collaborator

NightRa commented Aug 21, 2015

I personally don't think it's a good idea to break compatibility for this kind of name change.
Also, the name matching isn't any more intuitive than getOrModify.

@julien-truffaut
Copy link
Member Author

You're right I might have overlooked the argument of binary compatibility.

The good thing is that I believe that getOrModify is rarely used, so I don't think it would be too harmful to rename it in the next non binary compatible release (1.2.0). Btw, I don't know if you noticed but I let getOrModify but deprecated it.

matching talks more to me that getOrModify but that's probably personal.

@julien-truffaut
Copy link
Member Author

@NightRa do you think the aliasing + deprecation is acceptable or we should rather revert this change?

julien-truffaut added a commit that referenced this pull request Aug 31, 2015
@julien-truffaut
Copy link
Member Author

@NightRa I rolledback this change, I agree with you there is no point to make such a change now.

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.

2 participants