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

Added arc documentation #1049

Merged
merged 2 commits into from
Apr 19, 2022
Merged

Added arc documentation #1049

merged 2 commits into from
Apr 19, 2022

Conversation

pauljurczak
Copy link
Contributor

No description provided.

@jmwright
Copy link
Member

@pauljurczak Please merge CQ's current master branch into your fork to fix the failing CI checks.

@jmwright
Copy link
Member

@pauljurczak Thanks for the contribution.

Straight-forward doc change, merging with only one review.

@jmwright jmwright merged commit 6ae673a into CadQuery:master Apr 19, 2022
@lorenzncode
Copy link
Member

lorenzncode commented Apr 23, 2022

I would recommend to revert this doc change for following reasons (at least wait for special handling in the docs for multimethod).

  1. Sketch methods including arc, segment, spline make use of multimethod and now the docstrings are inconsistent.

    A docstring is defined on only the first of each of these methods currently.

    Prior to this change, the docstrings were generic.

    arc Construct an arc.

    segment Construct a segment.

    spline Construct a spline edge.

    After this change, the arc docstring is specific to the first multimethod where the segment, spline docstrings remain generic.

image

  1. It is inconsistent to add a specific docstring for the Edge.makeThreePointArc(Vector(p1), Vector(p2), Vector(p3)) implementation but not the other two registered variants.

  2. The original docstring IMO is preferred at this time because the HTML docs would require special handling if a docstring were to be added to each registered multimethod.

    Here is an example that shows the corrupt output with placeholder docstrings without special autodoc handling.

Screenshot from 2022-04-11 22-44-37

  1. Let's ignore the HTML docs - would adding 3 new docstrings to the arc multimethods help the user better understand these methods? IMO, no, it would add little value. If you are reading the source directly, it should become apparent from the signature and the definition itself. The first arc method is basically a single line of code, the second two lines, and the third a few more.

Perhaps after some work to implement special autodoc handling for multimethod, docstrings could be added to each registered method. For arc it could be something like:

Construct an arc through three points

Construct an arc from current endpoint and two points

Construct an arc given center, radius, and angles

Until then, manually created documentation/examples could cover the various methods.

@pauljurczak pauljurczak deleted the patch-2 branch April 23, 2022 20:47
@pauljurczak
Copy link
Contributor Author

pauljurczak commented Apr 23, 2022

I would recommend to revert this doc change for following reasons (at least wait for special handling in the docs for multimethod).

I was planning to document the other arc methods, too. I wanted to find out first if it affects the general documentation structure, which was/is unknown to me. You are making very good points that it is not consistent with its current structure.

would adding 3 new docstrings to the arc multimethods help the user better understand these methods? IMO, no, it would add little value

Whatever final shape the documentation takes, adding geometrically unambiguous descriptions to arc methods is necessary for newbies like me. Preferably reachable through HTML documentation search, not browsing through the source code.

@adam-urbanczyk
Copy link
Member

Hm, I'm agreeing with @lorenzncode on that one.

@jmwright
Copy link
Member

@lorenzncode Feel free to revert this for now. I won't be at a keyboard and able to do this until sometime on Monday.

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.32%. Comparing base (6217431) to head (252c9dd).
Report is 355 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1049   +/-   ##
=======================================
  Coverage   96.32%   96.32%           
=======================================
  Files          40       40           
  Lines        9407     9407           
  Branches     1248     1248           
=======================================
  Hits         9061     9061           
  Misses        204      204           
  Partials      142      142           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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