-
Notifications
You must be signed in to change notification settings - Fork 8
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
Proposal for new API #19
Comments
Might be nice to have a |
Nice to hear you are happy with the library! I'm always open to suggestions that make the library better. Let's discuss these, one at a time. ConstructingGreat suggestions! I like I've been struggling with all the Conceptually I see a difference between a
I would like to have both. I would also like to switch between those two naturally. Is this something that appeals to you? (If we can maintain an easy to use API, of course.) Or do you prefer the convenience methods? |
I see I have neglected an issue! Let's tackle that one after this one. |
I saw that difference as well. I think in most of our use-cases this would be solve with the functions I proposed in
I think I wouldn't like having this in the Zipper lib. I think it should only work with non-empty lists and I think the most expressive way to construct a We use |
Just wanted to quickly chime in that I like this proposal a lot. In particular I like the separate module with the
This is a cool concept! I wonder if it needs to be the same data structure though. Keeping both structures separate might keep both their implementations simpler. |
This will definitely be a different data structure! I'll try to implement this in a separate pull request and 1) see if I can easily maintain the current API 2) see if you guys like it above what we currently have.
I have thought about it some more and am a little worried people might interpret The functions append elems = last >> insertAfter elems
prepend elems = first >> insertBefore elems |
SGTM!
That would change the focus in the
I'm not super worried about people not understanding I'm more worried that people don't understand |
To me, a In my experience, but admittedly my experience is limited to hobby projects, all If the primary use case for P.S. I hope I don't come across as unwilling to implement your proposal, as it is very welcome. I just want to make sure we keep the library lean and focussed. |
Not at all! I was hoping to start a discussion with this and didn't expect you to just blindly go with it. Sorry was pretty busy the last week. Will take some time next week and reply and add some more thoughts. We don't have to hurry with this :-). |
Agreed and also but less the nonempty-properties that come with it.
Totally agree.
You might be right on this. I'm happy with |
Awesome! Let's start with |
What made you decide on the name |
We used this name for similar function in a different datastrucutre, but I don't have a strong opinion on the name maybe I think the usecase was something like: zipper
|> mapWith { before = (,) LT, current = (,) EQ, after = (,) GT } |
different thought I had: What if we would change next : Zipper a -> (Position, Zipper)
-- where Position is
type Positing = Already Position | Start | Inside | End or maybe a better solution would be to do: next : Zipper a -> Zipper a
-- and have a function that you could use before moving like
position : Zipper a -> Position
-- where Position is
type Position = Start | Between | End
-- so a user can choose to ignore the position and just call next regardless
-- or call `position` before moving. |
We could even make it generic in what the result of type Movement = Previous | Next
type alias Mover a r = Movement -> Zipper a r -> r
type Zipper a z = Zipper (List a) a (List a) (Mover a z)
next : Zipper a z -> z
next ((Zipper _ _ _ m) as z) = m Next z That way, a We might be burdening the users of the library a lot with this decision though. |
I even think we can implement a circular |
I've been playing with the idea of a
Any input? |
I've built custom behavior on top of the current With regards to the circular movement. Given a choice between this API: -- Move to the previous element
List.Zipper.previous zipper
-- Move to the previous element, wrapping around to the last element if none exists
List.Zipper.Circular.previous zipper And this API: -- Move to the previous element
List.Zipper.next Previous zipper
-- Move to the previous element, wrapping around to the last element if none exists
List.Zipper.next PreviousCircular zipper I think the former is much clearer. |
I totally agree with Jasper's comment ☝️ |
closing because of inactivity |
Proposal for new API
We've now used
List.Zipper
in many places of our application (87 modules depend on it).I've created a draft of an API that solves all our use cases, while hiding the implementation, reducing the api surface and hopefully making finding elements easier. This issue is meant as a starting point for a discussion about the api, I really like this package and the data-structure has proofen to be super useful. Thanks for all your work on this @wernerdegroot!
Type
type Zipper
a as an opaque type. (I totally agree with #10)Constructing
This allows for simpler construction of a zipper. No
Maybe
!In all our use-cases we want a zipper and we know that we have elements.
We added two different wrappers around
toList
andwithDefault
in our codebase to make this easier. I'm happy to keeptoList
though if you think it's useful.This might also address #15
Accessors
Same as before.
Mapping
Formatting everything at once.
We could provide a
defaultFormatter
with everything set toidentity
so a user can update that record with the one they wanna change.SelectList
hasmapBy
, but I kinda likeformat
better. http://package.elm-lang.org/packages/rtfeldman/selectlist/1.0.0/SelectList#mapByMoving around
New sub module:
List.Zipper.Circular
there is http://package.elm-lang.org/packages/maorleger/elm-infinite-zipper/2.1.0/List-InfiniteZipper, but it would be super useful (I've got one newer use-case) to have the same type for this.moveUntil
replaces allfind
functions.I hope you don't mind this proposal and I'm looking forward to discussing this.
The text was updated successfully, but these errors were encountered: