-
Notifications
You must be signed in to change notification settings - Fork 18
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
Improve stubs quality #23
Conversation
Oooh, yes of course. Defines all happen within the same translation unit, and since all of cmdc is one big giant translation unit, they will collide.
Yes, let's do it this way. Although I'm not 100% happy with using But, I can't think of anything better at this point, but keep an eye out for something even more readable and editable.
Yes, you also noticed haha.
Wait, are you saying the Perhaps an even better reason to think of another way.. |
Yup, the only way I've found where they don't end up in the docstring is this which isn't exactly better haha #define _doc_extendToShapeDirectlyBelow "This method is used if the end of the path is a transform and there are shapes directly below the transform.\n" \
"The shape to extend to is set by passing in an appropriate index parameter.\n" \
"Use the `numberOfShapesDirectlyBelow()` method to determine how many shapes are below\n" |
Ah yes, good find. That is better IMO, once you also indent the hanging lines, such that the line up vertically. This makes it very explicit what goes where. |
Sounds good, I'll go with that then 👍 |
That's mostly everything done here @mottosso let me know if this format for the docstrings suits you, I've set up the multiline docstrings like that, I felt it was more readable and would make more sense to judge the length of the lines though I haven't done any work to make them fit in 80 characters. just moved them as is. #define _doc_FnDagNode_getConnectedSetsAndMembers \
"Returns a tuple containing an array of sets and an array of the\n"\
"components of the DAG object which are in those sets. If the entire object is in a set, then the corresponding entry in the comps array will have no elements in it.\n" There are still 11 |
Ah, will need to setup the release properly as well |
It looks allright. There was one more alternative I had in mind, not sure what to think of it yet. #define _doc_DagPath_exclusiveMatrixInverse "Returns the inverse of exclusiveMatrix()."
#define _doc_DagPath_extendToShape "If the object at the end of this path is a transform \n" \
"and there is a shape node directly beneath it in the \n" \
"hierarchy, then the path is extended to that geometry \n" \
"node.\n" \
"NOTE: This method will fail if there multiple shapes \n" \
"below the transform."
#define _doc_DagPath_extendToShapeDirectlyBelow "This method is used if the end of the path is a transform and there are \n" \
"shapes directly below the transform.\n" \
"The shape to extend to is set by passing in an appropriate index parameter.\n" \
"Use the `numberOfShapesDirectlyBelow()` method to determine how many shapes \n" \
"are below" The trick being that we'll have to keep an eye out for making sure the lines are ~70 chars long, plus however long the indentation is. But this I think could help clean up that top section, and clearly separate the defines from the comment. What do you think? |
I feel like this is going to make keeping consistent line length awkward because of the differences in the |
Good point. Maybe this? #define _doc_DagPath_exclusiveMatrixInverse \
"Returns the inverse of exclusiveMatrix()."
#define _doc_DagPath_extendToShape \
"If the object at the end of this path is a transform \n" \
"and there is a shape node directly beneath it in the \n" \
"hierarchy, then the path is extended to that geometry \n" \
"node.\n" \
"NOTE: This method will fail if there multiple shapes \n" \
"below the transform."
#define _doc_DagPath_extendToShapeDirectlyBelow \
"This method is used if the end of the path is a transform and there are \n" \
"shapes directly below the transform.\n" \
"The shape to extend to is set by passing in an appropriate index parameter.\n" \
"Use the `numberOfShapesDirectlyBelow()` method to determine how many shapes \n" \
"are below"
That initial indent can help structure things, and now they are all equal regardless of variable length. |
Yeah I like that, best of both worlds 👍 |
@mottosso I think we're good now again, hoping the release modification works without testing |
Let's go! |
Follow up PR with #16 with the goal of improving the quality of the stubs
This mainly focuses on ensuring all the arguments are named properly instead of simply being named
arg0
,arg1
, etc.for that I'm simply using
py::arg("arg_name")
in order to let pybind know about the argument namesI'm also extracting the docstrings out of the function definition like you've started doing @mottosso to try and have some consistency.
I've noticed two mild issues with this so far
cmdc\src\MFnDagNode.inl(21): warning C4005: '_doc_child': macro redefinition
. I'm thinking of naming the variables like this instead to avoid redefinitions_doc_FnDagNode_child
.\
I haven't found a way in C++ to not use those characters.
Worst case scenario I can scrape them from the generated stubs before writing the file
An example of the stubs with the
\
left inOther than that, the stubs are looking pretty good, I just need to do that on the remaining files before merging