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

[Property values] Add append_values method for modifying values #392

Merged
merged 3 commits into from
May 14, 2019

Conversation

hkchekc
Copy link
Member

@hkchekc hkchekc commented May 7, 2019

Refer to Issue #382.

@hkchekc hkchekc requested a review from achilleas-k May 7, 2019 16:05
@coveralls
Copy link

coveralls commented May 7, 2019

Coverage Status

Coverage increased (+0.08%) to 89.427% when pulling 6f9e745 on hkchekc:val_append into d849bda on G-Node:master.

@jgrewe
Copy link
Member

jgrewe commented May 8, 2019

I guess we need a test case for that :)

nixio/property.py Outdated Show resolved Hide resolved
@achilleas-k
Copy link
Member

achilleas-k commented May 8, 2019

I was wondering if there was a more pythonic way to do this and I realised that, since values is a property with a getter and a setter, anything we do on the property directly should just work, assuming the operation works on the type returned by the getter.

So this doesn't work:

prop.values.append(10)

because values is a tuple and tuples don't have an append method, but this does

prop.values += (10,)

because you can concatenate tuples with +.

It's essentially equivalent to

v = prop.values
v += (10,)
prop.values = v

and our values setter already takes care of resizing when necessary.

self._h5dataset.shape = np.shape(vals)

I suppose it might still be useful for users to have an append_values() method. Though I think it could be simplified to just a + operation on the values property. Perhaps we should also call it extend() if it's going to support multi-value append. Could do both.

EDIT: The append/extend methods could probably check if the new data is the same type (or can be upgraded to the type) of the existing data. If users use a += though, it's up to them to keep the types in mind, which I think is fine, since concatenating values of different types is equivalent to assigning a mixed-value tuple to the property.values property and should follow the same rules.

nixio/property.py Outdated Show resolved Hide resolved
nixio/property.py Outdated Show resolved Hide resolved
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@achilleas-k achilleas-k merged commit af33c94 into G-Node:master May 14, 2019
@hkchekc hkchekc deleted the val_append branch May 21, 2019 13:16
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

Successfully merging this pull request may close these issues.

4 participants