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
Add option to export binary STL #1106
Conversation
lorenzncode
commented
Jun 17, 2022
- exporters.export add "ascii" (or "ASCII") options dict
- Shape.exportStl add "ascii" bool param
- fix corrupt docstrings/typos in shapes.py
* exporters.export add "ascii" (or "ASCII") options dict * Shape.exportStl add "ascii" bool param * fix corrupt docstrings/typos in shapes.py
Codecov Report
@@ Coverage Diff @@
## master #1106 +/- ##
==========================================
- Coverage 96.32% 93.90% -2.43%
==========================================
Files 40 25 -15
Lines 9672 5328 -4344
Branches 1280 979 -301
==========================================
- Hits 9317 5003 -4314
+ Misses 210 193 -17
+ Partials 145 132 -13
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Initially I thought I would clean up some of the docstrings since I was already touching shapes.py. I've removed the unrelated docstring cleanup changes to simplify this PR. |
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.
One minor suggestion to improve readability.
NB: I excluded tests from overage calculation to prevent false positive on the relative coverage delta.
Improve readability Co-authored-by: AU <adam-urbanczyk@users.noreply.github.com>
@lorenzncode which default value did you have in mind? |
@adam-urbanczyk I had left the default as ascii mostly to maintain the current behavior and defer the decision. If you agree, let's change to binary in master. If a problem is raised prior to the next release, it can be reverted back to ascii. |
I'm OK with changing to binary, let's see what others will say. |
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.
Thanks @lorenzncode
Feel free to merge after the change to binary as the default is made. |
Remove redundant test
The behavior of deprecated exportShape is unchanged. Binary STL format support is not added to exportShape. It continues to export ASCII format. |
@lorenzncode Is this ready to merge? |
@jmwright Yes, in summary: Provide binary STL export format option |
also tests excluded from coverage calculation |
@lorenzncode Thanks! |