Allow for empty "properties" subdict #32
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a long-ignored bug where some keys could "get lost" in the
__currsys__
if an "alias" but no "properties" is present in a yaml dict. This occurs e.g. in MICADO'sdefault.yaml
in the SCAO and MCAO modes, that don't (and probably don't need to) define any "properties". But thenNestedMapping.update()
defaults to simply updating key-value-wise.Devil's Advocaat
The alternative would be to introduce an empty "properties" in each of the yamls. But then people will forget that somewhere and we're back at the bug, so that's fragile. Also would probably require an update to multiple IRDB packages, which is annoying.
Rant
I'm generally not a fan of this "abusive" overloading of the
.update()
method. I think that method should only do what you'd expect from a standard dict (in fact, by inheriting fromcollections.abc.MutableMapping
, we wouldn't even need to define such a "standard".update()
method here...). The "special sauce" that's added here is also rather ScopeSim-specific and thus shouldn't really be inastar-utils
to begin with. Perhaps an extra method (.update_alias()
??) would be fine though. Same thing actually goes forUserCommands.update()
over in ScopeSim, although I started to mitigate that in the (as of writing this un-PR'd)fh/chainmap
branch...