marshmallow icon indicating copy to clipboard operation
marshmallow copied to clipboard

unbundle ordered-set

Open xsuchy opened this issue 2 years ago • 5 comments

This module was added in 2014, but it is available in PyPI for long time.

xsuchy avatar Oct 19 '21 20:10 xsuchy

This patch is actually in package in Fedora Linux for ages. No issue has been reported during that time.

xsuchy avatar Oct 19 '21 20:10 xsuchy

See #546. Looks like there are (were ?) several competing implementations. Let's make sure we pick the right one if we go on with this.

lafrech avatar Oct 27 '21 07:10 lafrech

Looks like none is packaged on Debian.

Since we don't use all features of an ordered set, and since performance doesn't matter that much on schema creation, I'm wondering if we even need an ordered set in the first place. But I've been giving it a quick try to see if we could replace this with a list or a dict and I have the feeling that it would take more code than the recipe we currently use.

lafrech avatar Oct 27 '21 09:10 lafrech

The other option can be:

try:
  from ordered_set import OrderedSet
except ImportError:
  from marshmallow.orderedset import OrderedSet

xsuchy avatar Oct 27 '21 09:10 xsuchy

    try:
      from ordered_set import OrderedSet
    except ImportError:
      from marshmallow.orderedset import OrderedSet

I'd rather settle one way or another than maintain both ways using the conditional import above.

While I like to avoid code duplication, this part of the code is small and stable and there doesn't even seem to be a consensus on a single implementation (#546). It is mostly a question of principle. The "no-duplication" principle meets the "don't go making a lib for any piece of code that is more than 10 lines long, this is not Javascript" principle.

Thinking this again, I totally respect the choice of Fedora devs, but I don't feel strongly in favour of this. I'm fine with the status quo.

Leaving this open for others to comment.

#1744 is partly related.

lafrech avatar Nov 08 '21 22:11 lafrech