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

[WIP] prepend for functional collection updates #131

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chrisandreae
Copy link
Member

So this is a minimal and backward-compatible change, but I don't think I like the API much. Help me bike-shed it?

@chrisandreae
Copy link
Member Author

Bikeshed?

@thefloweringash
Copy link
Contributor

backward-compatible change

I'm not yet seeing the part that makes this backward-compatible.

With the before and after expression I assumed we had a way to express exhaustive regions as a sort of gap-lock. We don't, and I'm not convinced the additional complexity would have any benefit to consumers.

The factoring of before and after into prepend + anchor does give us the desired prepend and decreases the set of invalid requests. Which part of this don't you like?

@chrisandreae
Copy link
Member Author

chrisandreae commented Apr 2, 2020

It's a long time ago, hard to recover exactly what I was thinking.

backward-compatible change

I'm not yet seeing the part that makes this backward-compatible.

I'm pretty sure the comment referred to the first commit, not the second "alternative" commit

The factoring of before and after into prepend + anchor does give us the desired prepend and decreases the set of invalid requests. Which part of this don't you like?

I think that's exactly what I liked about prepend and anchor option. What I didn't like as much was it wasn't backwards compatible.

So it was:
commit 1: don't touch existing before and after. Add new third option to allow prepending to the beginning of the list without an anchor, in the same way that without it it appends. Backward compatible, also ugly.

commit 2: get rid of before and after, replace them with an anchor, and prepend/append to the anchor or the list as a whole if no anchor is specified. Cleaner, but not backward compatible.

Which did you like? Did you have another idea instead?

@thefloweringash
Copy link
Contributor

I'm pretty sure the comment referred to the first commit, not the second "alternative" commit

Oh, oops. I just read the diff.

When using before/after the consistent approach is to introduce a handle for the start and end of the collection, probably in the form of a sentinel value. This means the prepend is expressed as before: $list_start. Previously we let append be the implicit default, but this would also be generalized to after: $list_end, and would leave the door open for talking about ranges like you can with actual elements.

I remain unconvinced that this level of expressiveness is a good idea. Could we go with the breaking change with a migration from the (currently) less expressive before/after? Just to ease deployments with existing clients.

@chrisandreae
Copy link
Member Author

I think I'm OK with that. Hopefully the migration in the fupdate code won't be too brittle.

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