Skip to content
New issue

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

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

Already on GitHub? # to your account

property values are returned as tuple, add append method #236

Closed
wants to merge 9 commits into from

Conversation

jgrewe
Copy link
Member

@jgrewe jgrewe commented Mar 6, 2018

addresses issues #221, #223, and #227.

The tuple return disallows to append arbitrary types of stuff to the value list. With this we would not need a custom class for this. Internally, it is still stored as a list.

Adding values is possible via the append method which then checks for dtype, etc.
It is also possible to append a property. In this case the values are appended. Implemented the merge function for this.

Please check if you like it. @JuliaSprenger, did you have more specific ideas for issue #221?

@jgrewe jgrewe force-pushed the values branch 4 times, most recently from c5d340d to dc30406 Compare March 6, 2018 22:28
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 71.103% when pulling 189099c on jgrewe:values into 2830914 on G-Node:master.

@JuliaSprenger
Copy link
Contributor

This looks good, however I noticed some unexpected behaviour.

  • When providing a tuple as value this is then stored as a single string element instead of a tuple. This might make sense eg when storing pairs of coordinates, but since the odml values are now also tuples this is a bit confusing.
  • Single values are not editable any more. Is there simple a way to edit single values without converting the tuple to a list, editing and setting this as property value?

@jgrewe
Copy link
Member Author

jgrewe commented Mar 8, 2018

see #237

@jgrewe jgrewe closed this Mar 8, 2018
@jgrewe jgrewe deleted the values branch March 8, 2018 09:54
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants