-
Notifications
You must be signed in to change notification settings - Fork 0
Add directory context #71
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 95.34% 95.43% +0.08%
==========================================
Files 15 16 +1
Lines 602 613 +11
==========================================
+ Hits 574 585 +11
Misses 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Code looks good.
I would prefer but do not insist on a more specific module name, e.g. with_directory/directory_context/cwdcontext/whatever. The spirit of the project is that different pieces are independent of each other, but this will lead to multiple context managers in the same file. In particular this may cause confusion since the exception context manager is already in a separate file.
Please add documentation. IMO the one-sentence header in the docstring is perfect, but please add a code example, e.g. by taking the unit test and adding two print statements (inside and after the context). The header and code example from the docstring should then be added to the readme (which is sorted alphabetically by module name)
Signed-off-by: liamhuber <[email protected]>
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 directly copy-pasted the example into the README in #72; IMO the existing doc works perfectly there, but feel free to modify it further if you'd like. Once it's mentioned in the readme I'm 100% happy, so I'm going to pre-approve here.
Add example to readme
Based on pyiron/pyiron_base#1916