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

FIX: Make GiftiMetaData.data a list proxy, deprecate #1127

Merged
merged 3 commits into from
Jul 26, 2022

Conversation

effigies
Copy link
Member

This is a follow-up to #1091 with things that should have been done there, including proxying actions on the GiftiMetaData.data internal list and deprecating the property altogether.

I added a comprehensive test that checks that attempting to use GiftiMetaData.data as one might have pre-4.0 works once again.

Verified

This commit was signed with the committer’s verified signature.
Michael-F-Bryan Michael Bryan
@pep8speaks
Copy link

pep8speaks commented Jul 26, 2022

Hello @effigies, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 nibabel.

Comment last updated at 2022-07-26 13:27:38 UTC

Verified

This commit was signed with the committer’s verified signature.
Michael-F-Bryan Michael Bryan
The way to modify a metadata object used to be manipulating the .data
list. Proxy calls to the list to ensure updates to the dict-like object
now.
@effigies effigies force-pushed the fix/gifti_md_list branch from 0425502 to fcd2816 Compare July 26, 2022 13:18
@effigies
Copy link
Member Author

@htwangtw Could I bug you for a review here?

@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #1127 (301e024) into maint/4.0.x (c90b75d) will increase coverage by 0.01%.
The diff coverage is 96.49%.

@@               Coverage Diff               @@
##           maint/4.0.x    #1127      +/-   ##
===============================================
+ Coverage        91.76%   91.77%   +0.01%     
===============================================
  Files              101      101              
  Lines            12380    12433      +53     
  Branches          2423     2429       +6     
===============================================
+ Hits             11360    11411      +51     
- Misses             693      694       +1     
- Partials           327      328       +1     
Impacted Files Coverage Δ
nibabel/gifti/gifti.py 87.78% <96.49%> (+1.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c90b75d...301e024. Read the comment docs.

Verified

This commit was signed with the committer’s verified signature.
Michael-F-Bryan Michael Bryan
These really should have been deprecated, instead of FutureWarning'd.
Keeping them around will cause confusion at best.
@effigies effigies force-pushed the fix/gifti_md_list branch from fcd2816 to 301e024 Compare July 26, 2022 13:27
@effigies
Copy link
Member Author

I have, somewhat tediously, looked through every instance of GiftiNVPairs I could find on GitHub, and there are many uses cases where people darray.meta.data.insert(0, GiftiNVPairs(...)), but fortunately nobody seems to be slicing or anything like that.

One thing that isn't addressed here is that we no longer offer anything but insertion order, so people who previously put insert(0, ...) will not get the same result out. It doesn't seem like a major crime, though.

Copy link
Contributor

@htwangtw htwangtw left a comment

Choose a reason for hiding this comment

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

The test looks comprehensive and I cannot think of anything else. LGTM

@effigies
Copy link
Member Author

Thanks!

@effigies effigies merged commit e795be6 into nipy:maint/4.0.x Jul 26, 2022
@effigies effigies deleted the fix/gifti_md_list branch August 31, 2022 13:37
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.

None yet

3 participants