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.

Sign up for GitHub

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

Jump to bottom

Replace use of os.path.join (which exhibits platform dependent behaviour) with posixpath.join where necessary #93

Merged
merged 1 commit into from
Jul 15, 2021
Merged

Replace use of os.path.join (which exhibits platform dependent behaviour) with posixpath.join where necessary #93

merged 1 commit into from
Jul 15, 2021

Conversation

nOOb3167
Copy link
Contributor

os.path.join is used to form an url-like path (forward slash separators).
However, os.path.join separates by backslash on windows

Instead of using os.path (which the os module sets as 'import posixpath as path' or 'import ntpath as path', depending on platform), import posixpath directly and always get the correct behaviour (forward slash separators).

Below is an example of the current behaviour:
'upload' action request from test_basic_external_adapter.py::test_upload_action_new_file:

{'actions': {'upload': {'expires_in': 900,
                        'header': {'x-foo-bar': 'bazbaz'},
                        'href': 'https://cloudstorage.example.com/myorg\\myrepo/abcdef123456?expires_in=900'},
             'verify': {'expires_in': 43200,
                        'header': {},
                        'href': 'http://giftless.local/myorg/myrepo/objects/storage/verify'}},

On windows, the actions.upload.href field contains a backslash (myorg\myrepo), as result of os.path.join use, and the test fails.

After this PR, forward slashes (myorg/myrepo) are generated.

Some call sites use os.path.join correctly (For instance, to form arguments to the open() function), and are not modified by the PR.

@nOOb3167
Use posixpath instead of os.path where appropriate
84d8a8c 
This guarantees that join() will be performed using forward slashes.
@shevron
Copy link
Contributor

shevron commented Jul 9, 2021

Hi, thanks for the contribution and sorry for the delay in reviewing it (I am now busy on other projects...). I see the problem, and the fix is good, but I'm also not a huge fan of using posixpath directly - it is undocumented, and that always makes me worry about using it directly because it might change in some future Python version.

Can I suggest that we create our own util function somewhere, to merge paths in a posix-compatible way? This function can either wrap posixpath (then if it ever breaks the fix will be easy), or even just use str.join (I'm not sure if this has any downside once catering for different platforms isn't an issue).

@nOOb3167
Copy link
Contributor Author

A 'Note:' box from the official os.path documentation states:

Note: Since different operating systems have different path name conventions, there are several versions of this module in the standard library. The os.path module is always the path module suitable for the operating system Python is running on, and therefore usable for local paths. However, you can also import and use the individual modules if you want to manipulate a path that is always in one of the different formats. They all have the same interface

Summarized:

  • The individual 'versions' (posixpath and ntpath) of the path module are included in the standard library - and so available regardless of operating system.
  • It is stated these individual modules can be imported and used.
  • It is stated these individual modules all have the same inteface (Same as os.path).

The source code of the posixpath module states that:

Some of this can actually be useful on non-Posix systems too, e.g.
for manipulation of the pathname component of URLs.

I believe that posixpath module is documented, including its interface. Further there is evidence its code was authored with this use case (URL component manipulation) in mind.

I guess it is your call on the approach to take.

@shevron
Copy link
Contributor

shevron commented Jul 15, 2021

Ok, fair enough. Let's merge it, if one day it fails one some Python version it will fail quickly and will be an easy fix. Thanks for contributing.

@shevron shevron merged commit 55a876e into datopian:master Jul 15, 2021
@nOOb3167 nOOb3167 mentioned this pull request Jul 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nOOb3167 @shevron