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

Consider making removal of empty arrays, object, and strings explicit rather than by default #131

Closed
polytypic opened this issue Oct 13, 2017 · 0 comments

Comments

@polytypic
Copy link
Member

polytypic commented Oct 13, 2017

One of the basic ideas in Partial Lenses has been to support removal of elements and propagating removal. The ability to remove elements by setting them to undefined has, IMHO, proven itself and at least I use it all the time.

However, a part of the removal support has been that once arrays, objects, and also strings, become empty they are removed, or replaced with undefined, by default. This can sometimes be handy, but I'm wondering whether optics expressions were, on average, simpler in case empty arrays and objects would not be removed by default? It seems that this by default removal of empty things is one of the most common gotchas with Partial Lenses (witnessed only by issue #117, but it has come up in codebases using Partial Lenses and personal communications) and also that quite often, possibly most of the time, one actually does not want to remove empty things.

Instead of saying e.g. [..., L.define([]), L.elems, ...] when you don't want the array to be removed if it becomes empty, you would say e.g [..., L.defaults([]), L.elems, ...] when you do want the array to be removed if becomes empty.

Of course, setting an element or property to undefined would still remove that element or property:

L.set('x', undefined, {x: 1, y: 2})
// `{y: 2}` --- just like previously

But in case it was last, the result would be an empty array or object:

L.set('last', undefined, {last: 1})
// `{}` --- previously `undefined`

Thoughts?

Grepping through codebases using Partial Lenses I have access to, it seems that porting to the changed semantics (without empty removal by default) would mostly consist of removing uses of L.define and L.required. It therefore seems like the "upgrade path" would be to simply make the breaking change and, in the same release, make L.define and L.required warn in cases where they are used with empty arrays, objects, or strings in ways that appear redundant. After the semantic change, uses of L.define and L.required could often just be removed and this would be guided by the warnings.

@polytypic polytypic added this to the 13.0.0 milestone Oct 13, 2017
polytypic added a commit that referenced this issue Oct 14, 2017
polytypic added a commit that referenced this issue Oct 14, 2017
polytypic added a commit that referenced this issue Oct 14, 2017
polytypic added a commit that referenced this issue Oct 14, 2017
polytypic added a commit that referenced this issue Oct 14, 2017
polytypic added a commit that referenced this issue Oct 14, 2017
polytypic added a commit that referenced this issue Oct 14, 2017
polytypic added a commit that referenced this issue Oct 14, 2017
polytypic added a commit that referenced this issue Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant