added missing properties to RhinoBrep #1459
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1459 +/- ##
==========================================
- Coverage 61.98% 61.96% -0.02%
==========================================
Files 208 208
Lines 22433 22437 +4
==========================================
- Hits 13904 13903 -1
- Misses 8529 8534 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if not not filepath.endswith(".step"): | ||
| raise ValueError("Expected file with .igs extension") |
There was a problem hiding this comment.
The double negation is confusing and the check here unexpectedly raises a ValueError with a message for '.igs' even though the method name implies a STEP file. Consider rewriting the condition to 'if not filepath.endswith(".step"):' and updating the error message accordingly.
| if not not filepath.endswith(".step"): | |
| raise ValueError("Expected file with .igs extension") | |
| if not filepath.endswith(".step"): | |
| raise ValueError("Expected file with .step extension") |
Copilot uses AI. Check for mistakes.
|
|
||
| @property | ||
| def is_infinite(self): | ||
| pass |
There was a problem hiding this comment.
The 'is_infinite' property currently uses 'pass', which results in no return value. It should raise a NotImplementedError to signal that this functionality is not implemented.
| pass | |
| raise NotImplementedError("Infinite check is not implemented for Rhino Breps.") |
Copilot uses AI. Check for mistakes.
|
@tomvanmele ping |
| return self._brep.SolidOrientation | ||
|
|
||
| @property | ||
| def shells(self): |
| raise NotImplementedError("Shells are not implemented for Rhino Breps.") | ||
|
|
||
| @property | ||
| def solids(self): |
There was a problem hiding this comment.
also OCC terminology. every solid is bound by a shell. not every shell is a solid...
| @staticmethod | ||
| def _export_brep_to_file(filepath, brep): |
There was a problem hiding this comment.
why not just make this a function independent of the class?
There was a problem hiding this comment.
would also switch the order of the params...
| @staticmethod | ||
| def _import_brep_from_file(filepath): |
There was a problem hiding this comment.
why not just make this a function?
Tracking progress in #1447 (comment)
Added some missing properties and alternative constructors to
RhinoBrep.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.