Do not ignore empty collections in ListEditor

[arv]

We used to ignore empty collections inside a ListEditor which meant that a list editor could not be used to create lists that contain empty lists, maps or sets.

Fixes #3868

[arv]

This passes on Travis... I'm seeing test failures on master locally though?

[aboodman]

LGTM. What test failure are you seeing?

[arv]

FAIL    github.com/attic-labs/noms/go/perf/suite

I have a slight memory of it being problematic 2 years ago

[arv]

Do we need Emptyable after this?

Not sure. Will look into it.

[aboodman]

I think we can remove the Emptyable interface, but leave the Empty() methods. See also: http://neugierig.org/software/blog/2019/11/interface-pattern.html

On Tue, Dec 3, 2019 at 8:20 PM Erik Arvidsson notifications@github.com wrote:

Do we need Emptyable after this?

Not sure. Will look into it.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/attic-labs/noms/pull/3869?email_source=notifications&email_token=AAATUBCETOBZDJK5XC7CM33QW5D4HA5CNFSM4JVDJY52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF336QA#issuecomment-561495872, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAATUBA2YG57ZORZORHICRLQW5D4HANCNFSM4JVDJY5Q .

[arv]

I'm still curious to what might break in Attic with this change. If I understand correctly the attic processing pipeline depended on this behavior. To me that seems like a breach of abstraction and the attic pipeline should have been able to deal with this.

[aboodman]

I mean attic doesn't exist anymore so it's irrelevant. But it sounds like Attic UI (or pipeline) was relying on the fact that removing the last item from a collection would also remove the collection itself from its parent, and so-on, recursively.

[aboodman]

You can land this whenever you're ready, thanks!