Skip to content

Conversation

effigies
Copy link
Member

@effigies effigies commented Jan 3, 2023

I noticed that we have almost an identical implementation of TemporaryDirectory as the stdlib (https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory), so I scheduled it for deprecation and removal.

With the coming chdir context manager, the remaining tools can be made convenience wrappers on stdlib 1-or-2 liners, so I went ahead and did it. IMO it's a bit cleaner to read a single function with the setup-yield-teardown structure than a two-or-three method class.

@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Base: 92.38% // Head: 92.40% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (27b757b) compared to base (9000943).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1172      +/-   ##
==========================================
+ Coverage   92.38%   92.40%   +0.01%     
==========================================
  Files          98       98              
  Lines       12257    12248       -9     
  Branches     2520     2524       +4     
==========================================
- Hits        11324    11318       -6     
+ Misses        612      611       -1     
+ Partials      321      319       -2     
Impacted Files Coverage Δ
nibabel/tmpdirs.py 100.00% <100.00%> (+7.89%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…raryDirectory

Move examples into __init__ to suppress warnings during doctests
Simplifying these tools as wrapped functions makes the logic easier to
read.
@effigies effigies force-pushed the rf/drop_custom_temporarydirectory branch from 295e5ee to 27b757b Compare January 3, 2023 06:24
Copy link
Contributor

@ZviBaratz ZviBaratz left a comment

Choose a reason for hiding this comment

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

LGTM.
If you'd like, I think using pathlib instead of os for everything besides chdir would be preferable.

@effigies
Copy link
Member Author

effigies commented Jan 3, 2023

From an API perspective, it's good to accept either str or Path, but maintaining existing return types is important. These will accept either input, but have been emitting strings. We would need to raise a FutureWarning to switch to returning Paths, which hardly seems worth it when people can already wrap with Path() if they want it.

Without that change, using the pathlib API internally seems like it would make this more complicated, not less.

@effigies effigies merged commit ea962d5 into nipy:master Jan 3, 2023
@effigies effigies deleted the rf/drop_custom_temporarydirectory branch January 3, 2023 16:01
@effigies effigies added this to the 5.0.0 milestone Jan 3, 2023
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.

2 participants