vberlier / nbtlib

A python library to read and edit nbt data.
MIT License
119 stars 16 forks source link

Preserve type on List.copy() and slicing #149

Open MestreLion opened 2 years ago

MestreLion commented 2 years ago

I just noticed List.copy() returns a plain list, as it doesn't override the copy method, and was about to create a PR implementing it. Then I realized this is a deeper issue, so I need your opinion before moving on:

I could do some research to see what numpy.ndarray and other similar list subclasses do, and also if there's any principle suggested in collections.abc.MutableSequence.

Meanwhile, what's your opinion on this? Interested in a PR to implement copy() and related methods in Compound and List?

MestreLion commented 2 years ago

Some research indicates slicing (and by consequence .copy()) should preserve type:

Given that, now I have a strong opinion that at least List|Compound.copy() should preserve type, and so should slicing. You OK for a PR implementing this?

vberlier commented 2 years ago

Yeah this can't be easily handled right now because lists and compounds inherit directly from the builtins instead of MutableSequence and MutableMapping, so you would need to manually override each method. Inheriting from the builtins has advantages but it seems that the pythonic way to do it is to go through the ABCs and wrap an underlying list or dict. However it could also be less efficient, and I'm not sure if ABCs play well with mypyc if we go down that route, so this needs further investigation.

MestreLion commented 2 years ago

I was planning on overriding each method anyway, no problem with that. You're already overriding __getitem__, I would just adjust it and add copy and whatever additional methods are needed.

Yeah, ABCs used to be the norm, but there are legitimate cases for subclassing the builtins direcly. It used to be a chore and have a million caveats, but not anymore, specially in Python 3 (and even more with Protocols, etc). The builtins do inherit from the ABCs, so you can already use isinstance(Compound, MutableMapping). So I don't think it's worth converting tags to plain ABCs. You already had the trouble to deal with the builtins caveats (and this issue would improve that)

vberlier commented 2 years ago

Hmmm, now I'm also wondering about other tags like numeric and strings. Should string slicing or methods like .lower() return String instances? Should mathematical operations also return wrappers? Currently things like Int(123) + Int(1) return a builtin int. I think it's worth considering if we end up implementing that for lists and compounds.

MestreLion commented 2 years ago

Hum, very interesting point!

Yeah, I believe they should preserve type too. Both str and bytes preserve type on .lower|upper() (and I assume related methods too), and it would make API much smoother to use on numericals