agilescientific / striplog

Lithology and stratigraphic logs for wells or outcrop.
https://code.agilescientific.com/striplog
Apache License 2.0
204 stars 69 forks source link

Lexicon looks like a `dict` but doesn't subclass `dict` #125

Closed Zabamund closed 2 years ago

Zabamund commented 3 years ago

We can access the categories in a Lexicon via dot notation:

lexicon.colour for example

but not with keying notation as with normal dict. so this breaks for instance:

lexicon['lithology'] TypeError: 'Lexicon' object is not subscriptable

If this is a design decision no problem, but I thought it would have been nice to be able to use dict methods on this object.

mtb-za commented 3 years ago

This is consistent with many of the major pieces in striplog: Component, Interval, Position, Lexicon are all inheriting from Object. It might be worth revisiting this decision and possibly making them extensions of dict or UserDict instead? This is a major change, so we will definitely test things carefully, but it might be worthwhile.

@bsburnham is working on some output issues (to_dict and then to_json) that might be easier if some of these were closer to being dictionaries, rather than just looking a lot like them, and mirroring some of their functionality, but not all.

Zabamund commented 3 years ago

The doctring of Lexicon states that:

A Lexicon is a dictionary of 'types' and regex patterns.

this is what made this a bit weird for me: if it's a dictionary, then why can't I use all dict methods? again if this is a design decision, no problem at all but then a smaller non-breaking change would be to change that docstring to reflect that. As for subclassing dict or a Collections.UserDict, Luciano Ramalho has a nice section about that in Fluent Python on page 80 where he says that:

It's almost always easier to create a new mapping type by extending UserDict rather than dict.

see text for explanation.

mtb-za commented 3 years ago

I have been looking at this over the last couple of days. Using a collections.abc.MutableMapping gets us a fair few dictionary methods for 'free'.

By inheriting from this instead of object, it looks like a fair amount of normal dictionary things Just Work(tm). For example: image This works even though there is no .items() method defined, for example.

Going to keep looking at this for the other classes, but it looks initially promising, at the least.

Alternatives are to inherit from either dict or UserDict. https://python.tutorialink.com/subclass-dict-userdict-dict-or-abc/ has a nice summary of these, but I am still not quite sure whether using the MutableMapping or UserDict is the best way forward. It may vary slightly from class to class as well.

kwinkunks commented 2 years ago

We decided not to implement this and instead changed the docstring so it doesn't sound like it's supposed to be a dict.