Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
S3 upload_file, download file to support path-lib objects #2259
S3 upload_file, download file to support path-lib objects #2259
Changes from 17 commits
ada17c6
ef141cb
b25cdff
fa917ec
86053f2
6b84c21
351a7a5
049b10a
6cf1f6c
aa4d6bf
ab9643b
eb43aaf
be73d95
6108d6f
0970707
38962ec
ff0af1b
7f67c6f
3d1bdc4
1d05811
39b183d
fc84da5
bc6cae2
6257495
2efe07e
5a7f0a8
ec0fde5
4ef8361
b5da19b
8a6365e
b12faf9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some test coverage for path-like objects that are not
pathlib.Path
.pathlib.PurePath
might be a suitable option.Considering how paths have a habit of causing trouble, I am also wondering if there are any other edge cases that we should cover here. Are you confident that things like the following will work correctly: relative paths, paths that contain
..
, Windows paths with drive letters, paths that contain symlinks.What about error cases. Is the behavior the same for a path-like object that doesn't exist and a string representation of a path that doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplify the basis by returning the
fspath
of the Path-like object which is a string. If there is an issue with the string, it would occur with or without this feature given that the original functionality only accepts a string. If the user messes up their Path-like object's path somehow, I think that's up to them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that with your current code there is no difference between integration-testing with a path-like object and integration testing with a string. However, one purpose of tests is to have them fail when a future code contributor changes the implementation in a way that you did not expect. That would be the purpose of having one integration test that uses a path-like object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add those as additional unit tests for path-like objects, but I will also add unit tests with those edge cases as regular strings as well.
Also, I already added an integration testing for a path-like object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is
os.path.normpath
needed here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested it now, but the keys can be messed up on Windows:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add
normpath
to ensure it is properly encoded independent of the system.