-
Notifications
You must be signed in to change notification settings - Fork 381
use Path from pathlib everywhere. drop path dep #1909
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1909 +/- ##
==========================================
- Coverage 95.74% 95.28% -0.47%
==========================================
Files 29 29
Lines 7827 7911 +84
Branches 1179 1214 +35
==========================================
+ Hits 7494 7538 +44
- Misses 192 212 +20
- Partials 141 161 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7a0b6cc
to
3bc82aa
Compare
tests/test_exporters.py
Outdated
self._exportBox(exporters.ExportTypes.SVG, ["<svg", "<g transform"]) | ||
|
||
exporters.export(self._box(), "out.svg") | ||
exporters.export(self._box(), Path("out.svg")) |
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.
@greyltc Will users have to import pathlib in their scripts now to be able to use exporters.export
? If so, that will break existing user-facing behavior.
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.
Yes! This change means that everything that points to a place on disk goes through pathlib.Path
, no more strings for that.
Luckily pathlib is built in (in python's standard library) though.
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.
If there are some API/user facing functions you'd like me to switch back to taking string inputs for file locations, let me know and I can switch them back.
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.
Hey, all path arguments shall be strings. So this change cannot change any interface. +1 getting rid of the external dep.
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.
all path arguments shall be strings
Where?
Of course the calls to the underlying opencascade library need to be strings (otherwise none of the tests in this PR would have passed). But is your statement here meant to mandate that the paths in the python code be strings everywhere as well?
- Before this PR: some of the python path objects were
str
, some werepathlib.Path
and some werepath.Path
. - After this PR: every path object is
pathlib.Path
, except for right as they get passed to the OCP lib calls, they get converted tostr
, just for those calls.
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 mean: the current API shall not change (i.e. not change to function/method signatures). What happens in the function/method bodies is a different story of course.
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.
Ok. So those 6 functions listed here (plus Assembly.save
) are the ones that need to remain as path string input objects, correct?
Would it be alright if I change them so they can take both str
and pathlib.Path
path object type inputs?
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 think more (Shape.exportStep
, ....), essentially I expect no (backward incompatible) signature change from this pr.
Sure, you could annotate it as Path | str
1ca0aba
to
80476cb
Compare
if isinstance(path, str): | ||
path = Path(path) |
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.
if isinstance(path, str): | |
path = Path(path) | |
path = Path(path) |
or
if isinstance(path, str): | |
path = Path(path) | |
path = Path(path) if isinstance(path, str) else path |
if self.obj: | ||
yield self.obj if isinstance(self.obj, Shape) else Compound.makeCompound( | ||
s for s in self.obj.vals() if isinstance(s, Shape) | ||
yield ( |
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 this changed?
|
||
@deprecate() | ||
def readAndDeleteFile(fileName): | ||
def readAndDeleteFile(fileName: Path): |
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.
Is this really needed? No changes to legacy functions please.
if isinstance(child.obj, Shape) | ||
else Compound.makeCompound( | ||
s for s in child.obj.vals() if isinstance(s, Shape) | ||
( |
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.
Again, why is this changed?
|
||
def write3mf( | ||
self, outfile: Union[PathLike, str, IO[bytes]], | ||
self, outfile: Union[Path, str, IO[bytes]], |
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.
Looking at this, shouldn't everything be typed as PathLike | str
?
# verify that the number of solids is correct | ||
self.assertEqual(len(obj4.solids().vals()), 5) | ||
|
||
obj4point5 = ( |
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 assume that you are smoke testing something here. Maybe add a comment
# %% export | ||
def test_export(): | ||
|
||
filename = Path("box.brep") |
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.
This is unnecessary
# smoke test for now | ||
with tmpdir: | ||
show(wp, interact=False, screenshot="img.png", trihedron=False, gradient=False) | ||
filename = tmpdir / "img.png" |
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 revert the change. AFAIK pathlib supports with
No description provided.