Coverage decreased (-0.007%) to 91.783% when pulling9835998 on CRiddler:loadsave-pathlib_compat into30c0bc5 on nipy:master.

☔ The latest upstream changes (presumably#611) made this pull request unmergeable. Please resolve the merge conflicts.

Why is the PR unmergeable?

It cannot be merged because there are conflicts with the master branch - see the web interface for this PR for links.

@CRiddler, do you think you can fix the conflicts?

Codecov Report

Merging#610 intomaster willincrease coverage by0.2%.
The diff coverage is93.75%.

@@            Coverage Diff            @@##           master     #610     +/-   ##=========================================+ Coverage    90.1%   90.31%   +0.2%=========================================  Files          96       96               Lines       11914    12171    +257       Branches     2125     2127      +2     =========================================+ Hits        10735    10992    +257  Misses        834      834               Partials      345      345

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatedebf173...60a4624. Read thecomment docs.

The conflicts have been resolved, however as stated earlier- this approach to resolving pathlib objects is not final nor foolproof. Essentially, we are checking if thefilename is aPathlike object which is only available in python 3.6+ and coercing that properly. If its not aPathlike then we're coercing the object to six.unicode type. What happens when the user passes in the filename asNone? Should we be coercing None to a string/unicode type? While the example is a corner case, it is still technically a case of an improperly handled input which can be fixed by a more in-depth filename handling procedure as discussed in the earlier comments.

I'm setting this milestone to 3.0. Once we're only dealing with Python 3.5+, that reduces the range of behaviors we need to accommodate. If you want to try for 2.5 and support Python 2'spathlib, we can do that.

Hi@CRiddler,@fepegar. By pushing off to Python3-only, I think we can be a lot dumber about this. I would say the logic can simply be:

filename=pathlib.Path(filename)

If we place this at the head ofload,save,to_filename andfrom_filename, then we can just work withPaths internally, converting tostr only when we hit functions that don't support them.

Sound like a reasonable approach? We would also want to add some tests where we pass in valid and invalid strings, paths, path-like objects, and non-path-likes.

Sounds reasonable to me.

Hello@CRiddler, 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 runflake8 nibabel.

Comment last updated at 2019-10-23 15:31:04 UTC

Hi@CRiddler, we're now requiring Python 3.5.1+, so we can resume work on this with simpler assumptions about the sort of objects we're going to be passed. I merged in master to resolve conflicts and get you going, but let me know how you want to proceed here.

Hi@effigies, if we do want to convert everything intopathlib.Path objects and deal with those internally then a good amount offilename_parser.py will have to be rewritten. Do we still want to try to move forward with pathlib internally? Or should we add in the method thatpandas uses to coerce pathlike objects to python3 string types?

Hmm. I haven't really scoped out where all we will need to make modifications to usePaths internally. I figured we'd find the entry-points, and follow the logic to see where things break.

But if we want to take the Pandas approach, the entry-points are still the first step, so I don't see these as competing approaches. If anything the Pandas-like one would be a good first pass, and then we could evaluate if we want to take the plunge with internalpathlib.Paths. There are a number of places where we take either filenames or open filehandles, so this could end up getting messy, and might not be worth it.

As a note, it looks like Pandas has simplified their check:https://github.com/pandas-dev/pandas/blob/325dd686de1589c17731cf93b649ed5ccb5a99b4/pandas/io/common.py#L131-L160
Their minimum Python is now 3.5.3, so it seems pretty safe to follow suit. I don't think we need to copy theexpand_user bit, though.

I agree in that coercing everything to strings at the entry points will be the easiest way to go. The updated pandas method looks like it'll work perfectly for us since__fspath__ is supported in 3.6+ and coercingpathlib.Path types tostr will support any 3.5 <= 3.6 versions.

I will update the branch with a function similar topandas approach (not sure how to give them credit for that though) and then add that function in the entry points you listed. We can see where to go from there? I can probably get around to this by the end of the week.

Cool. We have external licenses in COPYING, IIRC, so that and a link to the original code (with specific revision) should suffice. Our licenses are compatible.

@CRiddler Just a bump on this one. Let me know if there's anything I can do to help.

@CRiddler Just a bump on this. Let me know if there's anything I can help with. I'm going to aim to feature freeze 3.0 on October 28 (three weeks from Monday) so we can have a decent RC period.

Yep! This is still on my radar, classes started for me this past week so I've been a little busy. This is the first on my to-do list once I have a bit of free time, so I should have it done over the weekend.

I mean... also enjoy your weekend.

Hmm. Looks like your history got a bit messed up there.

I'd try rebasing ontofbaeacc. Should be pretty clean, as it's the merge commit that seems to be causing damage.

Hopefully that fixed, I always struggle when it comes to wrestling with git history. If it needs some more reworking, I may need some hand-holding to resolve the history issue.

Nope.

So the easy way to do this is just:

git rebase -i fbaeacc

That will open an editor, probablyvim ornano:

pick 6056014b update _stringify_path docpick b54926e3 update docstrings to accept str or os.PathLikepick 8a34a7a0 mghformat accept pathlib for filespec_to_file_mappick d6c96c02 tests pathlib compatiblepick cf631067 fix _stringify_path doc

That should be fine, so save and exit. Now you'll see:

Auto-merging nibabel/loadsave.pyAuto-merging nibabel/filename_parser.pyCONFLICT (content): Merge conflict in nibabel/filename_parser.pyAuto-merging nibabel/filebasedimages.pyerror: could not apply b54926e3... update docstrings to accept str or os.PathLikeResolve all conflicts manually, mark them as resolved with"git add/rm <conflicted_files>", then run "git rebase --continue".You can instead skip this commit: run "git rebase --skip".To abort and get back to the state before "git rebase", run "git rebase --abort".Could not apply b54926e3... update docstrings to accept str or os.PathLike

If you rungit status, you should see:

interactive rebase in progress; onto fbaeacc9Last commands done (2 commands done):   pick 6056014b update _stringify_path doc   pick b54926e3 update docstrings to accept str or os.PathLikeNext commands to do (3 remaining commands):   pick 8a34a7a0 mghformat accept pathlib for filespec_to_file_map   pick d6c96c02 tests pathlib compatible  (use "git rebase --edit-todo" to view and edit)You are currently rebasing branch 'loadsave-pathlib_compat' on 'fbaeacc9'.  (fix conflicts and then run "git rebase --continue")  (use "git rebase --skip" to skip this patch)  (use "git rebase --abort" to check out the original branch)Changes to be committed:  (use "git reset HEAD <file>..." to unstage)modified:   nibabel/loadsave.pyUnmerged paths:  (use "git reset HEAD <file>..." to unstage)  (use "git add <file>..." to mark resolution)both modified:   nibabel/filename_parser.py

So editnibabel/filename_parser.py as you would during a merge conflict (search for<<<< and resolve), then:

git add nibabel/filename_parser.pygit rebase --continue

You may be dropped into an editor again, giving you a chance to modify the commit message. You can just save and exit, to leave it unchanged. Then you might see:

[detached HEAD 2d37fb71] update docstrings to accept str or os.PathLike Author: Cameron Riddell <cameronriddell1@gmail.com> 1 file changed, 2 insertions(+), 2 deletions(-)The previous cherry-pick is now empty, possibly due to conflict resolution.If you wish to commit it anyway, use:    git commit --allow-emptyOtherwise, please use 'git reset'interactive rebase in progress; onto fbaeacc9Last commands done (5 commands done):   pick d6c96c02 tests pathlib compatible   pick cf631067 fix _stringify_path docNo commands remaining.You are currently rebasing branch 'loadsave-pathlib_compat' on 'fbaeacc9'.nothing to commit, working tree cleanCould not apply cf631067... fix _stringify_path doc

In this case, there's a commit that had no effect. So you can just

git rebase --continue

When you've iterated through all of the necessary changes:

> git statusOn branch loadsave-pathlib_compatYour branch and 'origin/loadsave-pathlib_compat' have diverged,and have 3 and 6 different commits each, respectively.  (use "git pull" to merge the remote branch into yours)nothing to commit, working tree clean

Since your GitHub branch has a lot of old stuff in it, instead of:

git diff origin/loadsave-pathlib_compat

I would suggest just comparing to the commit you rebased against:

git diff fbaeacc9

If that looks reasonable, go ahead and force-push:

git push -f

Great! Thanks for this.

Yay! Thank you guys for having worked on this.