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

occ_impl doc strings #491

Merged
merged 2 commits into from
Nov 30, 2020
Merged

Conversation

marcus7070
Copy link
Member

I've realised that there are a few useful methods in the occ_impl layer that I wasn't familar with. So I'm going through and adding docstrings to a few and fixing any inconsistent formatting or other small things I see.

@marcus7070
Copy link
Member Author

marcus7070 commented Oct 24, 2020

Before I get too far I thought I should just check, these methods don't have doc strings because no one has got around to it yet, correct? Not because they are supposed to be hidden from users or sphinx? Or because the OCCT docs are supposed to be the only source and we don't want to repeat this stuff in CQ?

edit: For example, Shape.ShapeType looks like it should be a private method and Shape.geomType is the user facing method. At the moment ShapeType doesn't show up on the sphinx docs and I'm thinking I'll leave a doc string off that one (since I think it should actually be a private method).

@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #491 (202b8af) into master (5b5d8e0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #491   +/-   ##
=======================================
  Coverage   93.65%   93.65%           
=======================================
  Files          30       30           
  Lines        5881     5881           
  Branches      626      626           
=======================================
  Hits         5508     5508           
  Misses        234      234           
  Partials      139      139           
Impacted Files Coverage Δ
cadquery/occ_impl/shapes.py 90.94% <ø> (ø)

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 5b5d8e0...408e4de. Read the comment docs.

@marcus7070 marcus7070 force-pushed the marcus7070/docs branch 2 times, most recently from 55960c9 to 90749e6 Compare October 24, 2020 07:53
@marcus7070
Copy link
Member Author

I tried to squash commits and something went horribly wrong. Successfully squashed with that second force push now.

@marcus7070
Copy link
Member Author

There might be a problem with sphinx auto type hints. When self is type hinted it does this:
screenshot2020-10-24-173549
Is that correct behaviour?

@marcus7070 marcus7070 marked this pull request as ready for review October 24, 2020 08:27
@marcus7070
Copy link
Member Author

I'm done for now. If I'm feeling masochistic again I'll try to do some more in another PR.

@adam-urbanczyk
Copy link
Member

Thanks @marcus7070 - I think it was due to all of the above reasons.

@marcus7070
Copy link
Member Author

I think it was due to all of the above reasons.

Ok then, if there are reasons to not have doc strings on any of these methods then please do remove them from this PR or reject it entirely. I don't mind; I did the work to familiarise myself with the methods, it's not wasted effort.

@adam-urbanczyk
Copy link
Member

Thanks @marcus7070 , I think we should merge it before 2.1. In the end it does not hurt to have better docstrings.

Copy link
Member

@jmwright jmwright left a comment

Choose a reason for hiding this comment

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

Other than my comment, this is ready to merge. Thanks @marcus7070

cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved
@adam-urbanczyk
Copy link
Member

Merging, thanks @marcus7070 .

@adam-urbanczyk adam-urbanczyk merged commit 3914bc3 into CadQuery:master Nov 30, 2020
@marcus7070 marcus7070 deleted the marcus7070/docs branch December 2, 2020 00:37
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.

None yet

3 participants