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

Make format_path behave the same for absolute paths #1341

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

lzach
Copy link
Contributor

@lzach lzach commented Jun 11, 2020

The CourseDirectory.format_path method behaves differently depending on whether escape is set to True or not. This is because when no escape is needed, the method uses os.path.join, but when escape is set to True, it uses str.join. For most cases this does not matter, but when the path supplied to format_path is absolute, it causes the escaped version to return a different path than the non-escaped one. This is because format_path bases the paths it returns on CourseDirectory.root (it returns join(root, supplied_path_formatted)), and os.path.join will discard all paths before the last absolute path (so os.path.join("/some/path", "/other/path") will return "/other/path"). The str.join version, however, does not work this way.

There're two ways this inconsistency could be solved. Either make the escaped version behave like os.path.join or make the not-escaped version behave like str.join. As os.path.join is the standard way to concatenate paths, it seems sensible to make both return according to os.path.join. I think there is some argument to be made for forcing format_path to respect CourseDirectory.root, though. Then we could be sure that all paths passed through format_path is in the same sub-directory (I don't know if this is an issue anyone is concerned with?) and since the arguments to format_path might be regular expressions, it's hard to say for certain whether a particular path is absolute or not.

With the experience I've had implementing a custom exchange that uses the format_path method, having both behave as os.join.path seems like the better option. Therefore I've submitted an implementation that tries to mimic os.path.join's behaviour in the escaped version.

@lzach
Copy link
Contributor Author

lzach commented Jun 11, 2020

I just saw that there are other path related pull requests and issues (i.e. #1327, #1177, #1326). Maybe the way paths are handled should get some more in-depth solutions?

@BertR BertR self-requested a review June 21, 2020 10:15
@BertR BertR added the bugfix label Jun 21, 2020
@BertR BertR added this to the 0.6.2 milestone Jun 21, 2020
Copy link
Contributor

@BertR BertR left a comment

Choose a reason for hiding this comment

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

LGTM

@BertR BertR requested a review from choldgraf June 25, 2020 21:10
@BertR BertR merged commit b5ae9f8 into jupyter:master Jun 26, 2020
BertR added a commit that referenced this pull request Mar 16, 2021
…lute paths with and

 without escape=True)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants