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 Zipper opaque. Add fromCons and from constructors #26

Merged
merged 5 commits into from Apr 20, 2019
Merged

Make Zipper opaque. Add fromCons and from constructors #26

merged 5 commits into from Apr 20, 2019

Conversation

AdrianoFerrari
Copy link
Contributor

Basic attempt to make Zipper private (#10). Added two new constructors as well, since it would probably be useful to have these.

@AdrianoFerrari
Copy link
Contributor Author

AdrianoFerrari commented Mar 28, 2019

Looking forward to discussion, @wernerdegroot . I think making Zipper private is an important API change. What would be the next steps to get this (or something similar) merged? Thoughts?

@wernerdegroot
Copy link
Owner

Hi @AdrianoFerrari , thanks for contributing. Although I haven't responded really quickly, I really appreciate you taking the time 👍

I would like to propose to add a few unit tests for the new functions.

Also, the fact that the Zipper constructors are now private is breaking the currently existing tests. Could you take a look at that too? Maybe it's a good idea to add a function to construct it from a beforeReversed, current and after so people can migrate easily? That could be useful for fixing the tests too!

@AdrianoFerrari
Copy link
Contributor Author

AdrianoFerrari commented Apr 1, 2019

Thanks for the feedback!
I'll try to bring the tests in line, and see about that "beforeReversedetc." constructor.

@AdrianoFerrari
Copy link
Contributor Author

Fixed the existing tests, still need to add unit tests for new functions.

I'm worried about introducing the fromBeforeReversed constructor (or whatever we'd call it), because it's bringing back the confusion that opaque zippers were meant to fix.

Anyone that was using the Zipper constructor directly would have been adding a List.reverse to the before term. Migration will be as easy as removing that reversal. I'd say that clarifying the docs for the from constructor would be enough to ensure smooth migration.

Thoughts?

@AdrianoFerrari
Copy link
Contributor Author

Added new unit tests, and doc clarification for from constructor.

Zipper [] x xs


{-| Construct a `Zipper` from before, current, after. The order is preserved, so `(from [1,2,3] 4 [5,6] |> toList) == [1,2,3,4,5,6]`.
Copy link
Owner

Choose a reason for hiding this comment

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

Nice! The comment should help a lot!

@wernerdegroot
Copy link
Owner

Thanks for your contribution. I'll release this directly!

@wernerdegroot wernerdegroot merged commit ab88697 into wernerdegroot:master Apr 20, 2019
@wernerdegroot
Copy link
Owner

Published as 4.0.0! Well done!

@AdrianoFerrari
Copy link
Contributor Author

Yay 🎉!
Thanks for the guidance and feedback.

@AdrianoFerrari AdrianoFerrari deleted the opaque-zipper branch April 20, 2019 13:29
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

2 participants