Skip to content
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

text() docstring update #813

Merged
merged 11 commits into from
Jul 19, 2021
Merged

Conversation

lorenzncode
Copy link
Member

* Remove following line from docstring (appears to be copy/paste error from extrude())

   extrude always *adds* material to a part.

* Swap order of :param distance: and :param fontsize: to match argument order

* Mention fontsize is in model units (fix #463)

* distance: mention distance is for both extrude/cut

* Update kind default in docstring as "regular" not "Normal"

* Add missing fontPath arg description

* Add examples of usage

  - Using an installed system font

  - Using a font at provided path (when font not installed on system)

    I tested with the free fonts from http://www.gust.org.pl/projects/e-foundry/tex-gyre

  - Cut text into solid

* Change docstring to fix autodoc-typehints issue where the Parameters section is duplicated (see screenshot)

If this fix looks good, I can follow up with a similar docstring fix for the duplicate Parameters issue on other methods.

current

docs_text_current_duplicate_parameters

update:

doc_text_update

    * Remove following line from docstring (appears to be copy/paste error from extrude())

       extrude always *adds* material to a part.

    * Swap order of :param distance: and :param fontsize: to match argument order

    * Mention fontsize is in model units (fix CadQuery#463)

    * distance: mention distance is for both extrude/cut

    * Update kind default in docstring as "regular" not "Normal"

    * Add missing fontPath arg description

    * Add examples of usage

      - Using an installed system font

      - Using a font at provided path (when font not installed on system)

        I tested with the free fonts from http://www.gust.org.pl/projects/e-foundry/tex-gyre

      - Cut text into solid

    * Change docstring to fix autodoc-typehints issue where the Parameters section is duplicated (see screenshot)
@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #813 (2d2cba3) into master (f422dfe) will decrease coverage by 0.03%.
The diff coverage is 99.09%.

❗ Current head 2d2cba3 differs from pull request most recent head 83b1aae. Consider uploading reports for the commit 83b1aae to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #813      +/-   ##
==========================================
- Coverage   95.17%   95.13%   -0.04%     
==========================================
  Files          32       32              
  Lines        7413     7489      +76     
  Branches      796      798       +2     
==========================================
+ Hits         7055     7125      +70     
- Misses        221      225       +4     
- Partials      137      139       +2     
Impacted Files Coverage Δ
cadquery/cq.py 91.42% <ø> (ø)
cadquery/occ_impl/solver.py 94.32% <97.14%> (+0.62%) ⬆️
cadquery/assembly.py 93.13% <100.00%> (+0.40%) ⬆️
cadquery/occ_impl/shapes.py 92.46% <100.00%> (+0.02%) ⬆️
tests/test_assembly.py 100.00% <100.00%> (ø)
tests/test_cadquery.py 99.20% <100.00%> (+<0.01%) ⬆️
cadquery/cqgi.py 79.09% <0.00%> (-2.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f422dfe...83b1aae. Read the comment docs.

@jmwright
Copy link
Member

jmwright commented Jul 6, 2021

@marcus7070 Do the examples in this PR fall under the discussions you've been having about keeping examples in the docs up-to-date?

@marcus7070
Copy link
Member

Hmmmm, that's an interesting thought @jmwright. You could do a basic smoke test by using pytest's doctest integration and changing the docstring to:

 Examples:
>>> cq.Workplane().text("CadQuery",5,1)

Specify the font (name), and kind to use an installed system font:
>>> cq.Workplane().text("CadQuery",5,1,font="Liberation Sans Narrow", kind="italic")

Specify fontPath to use a font from a given file:
>>> cq.Workplane().text("CadQuery",5,1, fontPath="/opt/fonts/texgyrecursor-bold.otf")

Cutting text into a solid:
>>> cq.Workplane().box(8,8,8).faces(">Z").workplane().text("Z", 5,-1.0)

But I hate doing double duty (document and test) with docstrings like this because cq.Workplane().text("CadQuery",5,1, fontPath="/opt/fonts/texgyrecursor-bold.otf") is a good example but would fail in our test suite. That one isn't too hard to fix, but similar issues would crop up in lots of other docstrings. I'd rather let docstrings do one thing and do it well.

It's also of limited benefit since it's pretty rare a docstring goes out of date with the function, since they're right next to each other.

So my vote is for no tests in docstrings.

Copy link
Member

@marcus7070 marcus7070 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only important change is that :param self: one, the rest are trivial stuff but I was suggesting changes anyway so I thought I'd throw them in.

Thanks for doing this @lorenzncode!

cadquery/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
lorenzncode and others added 10 commits July 7, 2021 12:57
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Copy link
Member

@marcus7070 marcus7070 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this @lorenzncode, even if we didn't get everything done it's still an improvement.

@adam-urbanczyk
Copy link
Member

Thanks @lorenzncode ! Shall I merge @marcus7070 @jmwright ?

@jmwright
Copy link
Member

+1 to merge

@adam-urbanczyk adam-urbanczyk merged commit 8dc1a5c into CadQuery:master Jul 19, 2021
@lorenzncode lorenzncode deleted the text-docstring branch November 6, 2021 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants