-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
Path support #787
Path support #787
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #787 +/- ##
==========================================
+ Coverage 96.09% 96.10% +0.01%
==========================================
Files 25 25
Lines 8472 8481 +9
==========================================
+ Hits 8141 8151 +10
+ Misses 331 330 -1 ☔ View full report in Codecov by Sentry. |
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.
For my education, what does the input type bytes
actually do here? Why would a user pass bytes
- is it for unicode file names?
There is no need to refactor the deprecated export methods. |
For all import / export methods make the path a `Union[Pathlike, str, bytes]` argument to follow python conventions as outlined in pep 519
I am not 100% sure. It seems to be a leftover from the fact that the c standard library used raw byte strings to represent paths. There is some discussion in pep 519 here. I included it in the signature because all of the underlying
|
Thank you! Another nice improvement. |
Thank you! I've been looking forward to this very small quality-of-life improvement :) |
For all import / export methods make the path a
Union[Pathlike, str, bytes]
argument to follow python conventions as outlined in pep 519.closes: #685
closes: #774
I got carried away big time and this PR is probably unreasonably large. To test the new functionality I changed all the tests to feed inpathlib.Paths
. In addition any tests which wrote to disk were rewritten to use the pytesttmp_path
fixture. Which then caused me to rewrite the tests in the pytest style...I can revert the test changes and be less invasive with my changes if desired.Edit: I decided to roll those back. I think they're good changes in the long run but they get in the way of reviewing the pr at hand.
Open questions:
topology.py
also be moved over to this scheme? Technically that is what Feature Request: Add support for exports topathlib.Path
:.export_stl(path_object)
#685 is requestingshould the tests attempt to verify that all three types: string, paths, and bytes work?I added said tests