-
Notifications
You must be signed in to change notification settings - Fork 291
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
Implement locations() for Wires #475
Conversation
Giving locations on wires would be a great help on placing objects around the perimeter of a face. Two comments:
|
|
Codecov Report
@@ Coverage Diff @@
## master #475 +/- ##
==========================================
+ Coverage 93.63% 94.22% +0.59%
==========================================
Files 30 30
Lines 5859 7848 +1989
Branches 624 942 +318
==========================================
+ Hits 5486 7395 +1909
- Misses 234 289 +55
- Partials 139 164 +25
Continue to review full report at Codecov.
|
|
@jmwright if everything passes I'd say it is ready for merging. |
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.
@adam-urbanczyk Thanks for doing this. I just have a few minor comments.
Currently Perhaps that's intentional, but I thought I'd point it out incase it's an oversight. |
Good catch, it was not the intention. I'll add |
I tried to add |
@Peque Do you have any ideas on the Spinx part of this (see previous comment from @adam-urbanczyk )? You've done quite a bit of work with Sphinx. |
@jmwright No idea, sorry. 😅 I may be able to have a look at it if you are still blocked in 1-2 weeks. Sounds like a good question for StackOverflow though. 😜 |
It works now, but could be prettier. I propose to merge and maybe investigate autodoc alternatives in the future. @jmwright would you agree? |
+1 to merge |
Implements
locations()
andlocationAt()
for both Wires and Edges.