-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update Pathlib usage and docstrings #68
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
- Coverage 90.56% 90.54% -0.02%
==========================================
Files 35 35
Lines 1356 1364 +8
==========================================
+ Hits 1228 1235 +7
- Misses 128 129 +1 ☔ View full report in Codecov by Sentry. |
CodeCov doesn't like my Happy to change those to regular imports on reviewer's request to avoid the red-cross-of-doom 😅 |
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.
Looks good to me! Just a few minor changes I noted in the comments. For the TYPE_CHECKING
imports I wonder if there's a way to exclude these lines from codecov
as coverage
can do? Otherwise, probably best to just use standard imports to avoid the red cross 😅
Co-authored-by: Kimberly Meechan <[email protected]>
for more information, see https://pre-commit.ci
Went for the straight-up import solution 😅 The solution given in the linked issue seems to imply that the |
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.
Looks good! For consistency, I just highlighted a few areas where quotes are no longer needed around the types.
All the "Path"s Co-authored-by: Kimberly Meechan <[email protected]>
VSCode linter why have you forsaken me 😭 |
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.
Looks good! I'll go ahead and merge now.
Description
What is this PR
Why is this PR needed?
Standardises our imports of
pathlib
Path
s and avoids importing the whole module when we only need thePath
class.What does this PR do?
Path
tofrom pathlib import Path
across the package.Path
inputs.References
No issues, but picked up on in a comment on #65 (comment)
How has this PR been tested?
Tests pass locally. No package functionality should be changed, either.
Is this a breaking change?
No.
Does this PR require an update to the documentation?
N/A
Checklist: