Improve compatibility with pathlib by implementing more common methods #230
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I added a few more compatibility changes:
|
pjbull
requested changes
May 30, 2022
There was a problem hiding this comment.
Awesome PR, thanks @Gilthans !
In addition to the comments, could you:
- (1) make linting pass (
make formatthenmake lintshould check this for you) - (2) update the README.md so the table reflects the new supported methods
- (3) add a note to HISTORY.md with the updates
Codecov Report
@@ Coverage Diff @@
## master #230 +/- ##
========================================
- Coverage 94.7% 94.4% -0.4%
========================================
Files 21 21
Lines 1287 1323 +36
========================================
+ Hits 1219 1249 +30
- Misses 68 74 +6
|
pjbull
requested changes
May 31, 2022
There was a problem hiding this comment.
A couple documentation tweaks, and then testing for the cloud_prefix and this should be good to go.
For the failing docs build in CI, can you update requirements-dev.txt with a change like the one we had to make in another project?
https://github.com/drivendataorg/nbautoexport/pull/96/files
pjbull
reviewed
May 31, 2022
|
Thanks @Gilthans! Moving this on to a contributor branch to run the live tests. |
pjbull
added a commit
that referenced
this pull request
Jun 1, 2022
#230) (#232) * Added absolute, resolve and relative_to implementations * Added tests interacting with PurePosixPaths * Added is_absolute and is_relative_to as well * Accept read_text parameters * Added exist_ok paramter to touch * Added missing_ok paramter to unlink * Fixed formatting (make format) * Update history.md & readme.md * Fix relative_to on different providers (+ some naming changes * Reformat * Update history.md and bump version number to 0.9 * Update dev requirements for docs * Fix readme list order * Added test for relative_to on different provider cloud paths * Remove unused import * Updated relative_to different cloud test Co-authored-by: Daniel Rembiszewski <[email protected]>
|
Merged into the main branch, thanks @Gilthans ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
#149, #151
absoluteandresolve, which do nothing. This allows the methods to be called when the path type is unknownrelative_to, which returns a relative PurePosixPathis_absoluteandis_relative_toread_textparametersexist_okparameter totouchmissing_okparameter tounlink. Even though it defaults to False in pathlib, adding it with that default would be a breaking change, since current implementation doesn't check for existence, so I added it defaulting to True.