Tickets/dm 5643

[pschella]

This pull request does three things.

1. Add .toDict() method to PropertySet
2. Add .toOrderedDict() method to PropertyList

Unit tests are included for both items and a CI build has succeeded.

[PaulPrice]

Is it necessary to spell it toOrderedDict instead of just toDict (as a natural override of the base class' method)?
Otherwise, awesome!

[ktlim]

Looks good to me. I'm a little worried that the tests don't appear to test the case that a list/vector of values is associated with a single property in a PropertySet, but this is a rare use case anyway. I take no position on the naming that Paul suggested.

[RobertLuptonTheGood]

Is the point of this to make a PropertyList look like an OrderedDict? If so, why don't we just add the methods to duck type it to quack like an OrderedDict?

[ktlim]

I can think of at least two reasons: 1) it might be a lot of methods (haven't looked), and 2) there are differences in the semantics (e.g. the current toOrderedDict test loses the first "COMMENT").

[RobertLuptonTheGood]

As @timj moves us towards more pythonic interfaces, surely we need to move in this direction (at least at the python level). We would have made PropertyList look like a dict if we designed it now.

[timj]

I have to admit I was wondering why we weren't just adding all the relevant dunder methods to make it quack like a dict (via UserDict). Rather than having a method to return a separate dict.

[TallJimbo]

I think the PropertSet handling of multi-valued keys is sufficiently different from dict's (lack of) handling that we'd have to make breaking changes to PropertySet in order to make it just behave like a dict. I think that's probably worth doing eventually, but I didn't want to do it now. That's partly because I think designing an object that both behaves naturally like a dict and can round-trip FITS headers (which we need PropertyList to do) is really hard when you get into the details,

[timj]

https://github.com/timj/perl-Astro-FITS-Header 😄 implements a dict interface and can round trip to FITS.

[TallJimbo]

Oh, it's not impossible; astropy.io.fits.Header does it too. But I believe many earlier versions of that class didn't get it right.

[pschella]

@PaulPrice, the spelling as toOrderedDict is not necessary. However, it might surprise users if toDict returns an OrderedDict instead. That said, there is already a rather ugly difference between the behaviour of PropertySet and PropertyList. Namely that nesting the former properly nests, whereas the latter produces a flattened version (e.g. if a PropertySet ps2 with property b is inserted into ps1 then the value can be accessed via ps1["ps2"]["b"], but for PropertyList it would be pl1["ps2.b"]). now that can obviously be fixed, but that would make it even more incompatible with the preexisting toList (as well as the respective get methods).
I'm afraid these classes just don't behave very Pythonic.

[pschella]

Please note that I just pushed a commit to generate the OrderedDict without feeding the output of toList to the OrderedDict constructor. This is needed because toList expands array properties into separate list items with identical names.
Technically this change makes the addition of the comments argument to toList obsolete, but it still might be nice to have?

[PaulPrice]

Thanks for the explanation @pschella. You're probably right to spell them differently.