Skip to content

ENH: Allow ~/ in paths #1260

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

Merged
merged 9 commits into from
Oct 4, 2023
Merged

ENH: Allow ~/ in paths #1260

merged 9 commits into from
Oct 4, 2023

Conversation

ReinderVosDeWael
Copy link
Contributor

This PR modifies _stringify_path to allow it to expand home directories (~/). Its output should remain the same on all other valid inputs.

Tests are included to validate a variety of cases. As no prior tests existed for this function I cannot confirm that its behavior remains identical in all other cases.

@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4e7ad07) 92.19% compared to head (cca2b4a) 92.19%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1260      +/-   ##
==========================================
- Coverage   92.19%   92.19%   -0.01%     
==========================================
  Files          99       99              
  Lines       12458    12457       -1     
  Branches     2560     2559       -1     
==========================================
- Hits        11486    11485       -1     
  Misses        648      648              
  Partials      324      324              
Files Coverage Δ
nibabel/filename_parser.py 94.18% <100.00%> (-0.07%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

I don't think there's any advantage to forcing to POSIX apart from to please the tests. I would just us os.path.join to ensure the tests are consistent with the OS.

It also won't work as-is, since Path.home() will contain backslashes in Windows.

@ReinderVosDeWael
Copy link
Contributor Author

Some of the failures were in other, existing, tests, would you want these modified accordingly?

@ReinderVosDeWael
Copy link
Contributor Author

See test_parse_filename in the same test file as an example.

@effigies
Copy link
Member

Oh, I see. Well, if we're munging plain strings instead of passing them through untouched, then I suppose it doesn't matter much whether they become POSIX-style or match os.sep.

I'm trying to think if there is any other action at a distance that this will cause. I think img.get_filename() will now become normalized to whatever we choose, which should only affect someone who is actually comparing that to the paths they loaded. That seems pretty unlikely...

@ReinderVosDeWael
Copy link
Contributor Author

Also apologies for the repeated test pushes; I don't have access to a windows machine so I can't test locally

@effigies
Copy link
Member

effigies commented Oct 3, 2023

@ReinderVosDeWael I assume you saw that a new thing has broken:

___________________________ TestEcatImage.test_file ___________________________
[gw1] win32 -- Python 3.9.13 D:\a\nibabel\nibabel\virtenv\Scripts\python.exe

self = <nibabel.tests.test_ecat.TestEcatImage testMethod=test_file>

    def test_file(self):
>       assert self.img.file_map['header'].filename == self.example_file
E       AssertionError: assert 'D:/a/nibabel...ata/tinypet.v' == 'D:\\a\\nibab...ta\\tinypet.v'
E         - D:\a\nibabel\nibabel\virtenv\lib\site-packages\nibabel\tests\data\tinypet.v
E         ?   ^ ^       ^       ^       ^   ^             ^       ^     ^    ^
E         + D:/a/nibabel/nibabel/virtenv/lib/site-packages/nibabel/tests/data/tinypet.v
E         ?   ^ ^       ^       ^       ^   ^             ^       ^     ^    ^

..\virtenv\lib\site-packages\nibabel\tests\test_ecat.py:186: AssertionError
____________________________ test_mgh_load_fileobj ____________________________
[gw0] win32 -- Python 3.9.13 D:\a\nibabel\nibabel\virtenv\Scripts\python.exe

    def test_mgh_load_fileobj():
        # Checks the filename gets passed to array proxy
        #
        # This is a bit of an implementation detail, but the test is to make sure
        # that we aren't passing ImageOpener objects to the array proxy, as these
        # were confusing mmap on Python 3.  If there's some sensible reason not to
        # pass the filename to the array proxy, please feel free to change this
        # test.
        img = MGHImage.load(MGZ_FNAME)
>       assert img.dataobj.file_like == MGZ_FNAME
E       AssertionError: assert 'D:/a/nibabel...data/test.mgz' == 'D:\\a\\nibab...ata\\test.mgz'
E         - D:\a\nibabel\nibabel\virtenv\lib\site-packages\nibabel\tests\data\test.mgz
E         ?   ^ ^       ^       ^       ^   ^             ^       ^     ^    ^
E         + D:/a/nibabel/nibabel/virtenv/lib/site-packages/nibabel/tests/data/test.mgz
E         ?   ^ ^       ^       ^       ^   ^             ^       ^     ^    ^

Possibly the better check for these things is to assert that Path(x) == Path(y)?

@ReinderVosDeWael
Copy link
Contributor Author

Thanks for the ping - I had not seen this. I'll see if I can find a couple minutes later today (EST timezone) to resolve this.

@ReinderVosDeWael
Copy link
Contributor Author

That should do it; I've changed the failing tests to check for pathlib.Path equality. Again I can't test it on windows, so I'll await the CI.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Two small cleanups to the tests, but this is good to go.

@effigies
Copy link
Member

effigies commented Oct 4, 2023

Thanks! Hopefully this makes people's lives a little easier...

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.

3 participants