Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getitem of sections should treat props and sections identically. #240

Open
matham opened this issue Feb 6, 2017 · 3 comments
Open

getitem of sections should treat props and sections identically. #240

matham opened this issue Feb 6, 2017 · 3 comments

Comments

@matham
Copy link
Member

matham commented Feb 6, 2017

There are some inconsistencies, I believe, in sections/properties.

When accessing an item from a section by key, e.g. section['name'], the returned value is treated differently depending whether the key is a property or a section as can be sen here.

If it's a section, the section is returned. If it's a prop, if it's a single Value prop, the values[0].value is returned, otherwise a the Value's, values are returned. It think in the zen of python that explicit is better than implicit, we shouldn't do that.

At best, I think we should always return the Value's, values, even if there's only one. We shouldn't guess that a single valued prop is different than multiple valued prop. Unless the underlying code will always treat a single value different than multiple values (i.e. ValueList and Value, although this seems like a bad idea).

Or in my opinion, we should never return the Value's, values, but rather the Value itself. Just like we return the Section if it's a section. This would even allow us to do if isinstance on the returned value.

When assigning to a section or property(?) we can do section['name'] = 'stuff' which creates a single valued Value, so I can see the desire to also return the first item of the Value if there's only one. But I think users will be able to distinguish between allowing that assignment for ease of use, and always getting a Value when doing getitem, especially if it's documented. On the flip side, guessing what the user wants will lead to confusion, even if documented.

I believe that method should look as follows:

    def __getitem__(self, key):
        if key not in self.props and key in self.sections:
            return self.sections[key]
        return self.props[key]
@gicmo
Copy link
Member

gicmo commented Feb 7, 2017

I am myself no longer in the business of writing odML related code so I am staying out of this, and I do understand your argument, but I remember that I was the one pushing for this (and implementing it), mainly because I found working with odML very cumbersome and verbose, especially due to the multi-value support: just look at example in Java. This translated to python, with the least "implicit" as possible this would also look like foo.properties['property'].values[0].value. I personally found this to very ugly. As a side note, pyhton violates its on "zen" in so many places and there is also "Beatiful is better then ugly" (which I think applies here :)) and "Readability counts" and ....

@achilleas-k
Copy link
Member

achilleas-k commented Feb 7, 2017

I'm all for reducing ambiguity and I agree there's a bit of an issue here. With the harmonisation of odML and NIX that we're currently working towards, the support for multiple values will be dropped. So I think this will become a simpler issue to address then and it's not worth discussing how we want the current state to behave.

So the question now becomes whether __getitem__ on a Section should return a Value object or the value of the Value (this is getting confusing).

@matham
Copy link
Member Author

matham commented Feb 14, 2017

Once support for multiple values is dropped I think whether we should return Value or Value.value depends on this: Currently for len and del item we operate just on props. For items and contains we check both props and sections. For consistency we should either 1) always iterate over just props, or 2) always iterate over props and sections.

So for option 1), I think we should everywhere only iterate on props since generally I imagine when you iterate on a section you want to iterate over its properties not also its sections. If you also iterate on the sections you'd need to do a test to check whether what you received is a section or a property with e.g. isinstance, so you might as well explicitly iterate of the section's props and sections separately which would save you a isinstance.

So for option 1 I think we should return Value.value since that would be the most convenient. And since it iterates only on props this should be fine. For option 2, if we iterate over props and sections I think we should return the Value itself, that way when iterating you'd be able to know if you have a Property or Section.

I prefer option 1 since it's probably what most want by default, I think.

I think the desire behind what lead to iterating over props and sections currently, might be better served by having an explicit walk method which iterates through the section/property tree in a breadth or depth first manner. Perhaps it could have a depth parameter which when e.g. set to 1 would walk props and sections like current behavior. Or it could be like os.walk that returns the list of properties (filenames) for every section (directory)?

For more things to do, we have not talked about __setitem__ 💡. __setitem__ accepts a S, a value, or a list of values and creates a section or properties, respectively. Now that we're dropping support for multiple values it think it should act as follows. Instead of, or in addition to accepting S (not sure the significance of S as it seems to be only internal?) it should also except a dict. This would create a section with name key whose properties are the keys/values from the dict. This way we can create a section/property hierarchy easily though dict assignment.

I can work on implementing these things when we agree on how it should be. Also, what's the timeline on dropping support for multiple properties (although I guess we don't have to wait for that)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants