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

Improve stubs quality #23

Merged
merged 15 commits into from Jun 10, 2021
Merged

Improve stubs quality #23

merged 15 commits into from Jun 10, 2021

Conversation

Muream
Copy link
Contributor

@Muream Muream commented Jun 9, 2021

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 names

I'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

  1. On windows I get warnings like this 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.
  2. On multiline docstrings, the stubs end up with the \
    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 in
class DagPath():
    def extendToShapeDirectlyBelow(self, index: int) -> None: 
        """
        This method is used if the end of the path is a transform and there are shapes directly below the transform.\
        The shape to extend to is set by passing in an appropriate index parameter.\
        Use the `numberOfShapesDirectlyBelow()` method to determine how many shapes are below
        """

Other than that, the stubs are looking pretty good, I just need to do that on the remaining files before merging

@mottosso
Copy link
Owner

mottosso commented Jun 9, 2021

On windows I get warnings like this

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.

I'm thinking of naming the variables like this instead to avoid redefinitions _doc_FnDagNode_child.

Yes, let's do it this way. Although I'm not 100% happy with using #define, we're forced to include those aweful backslashes, which make it less inviting to actually edit that docstring and make it better. In this form, I expect we'll simply leave them as is for all time which is bad.

But, I can't think of anything better at this point, but keep an eye out for something even more readable and editable.

On multiline docstrings, the stubs end up with the \

Yes, you also noticed haha.

Worst case scenario I can scrape them from the generated stubs before writing the file

Wait, are you saying the \ gets included in the actual docstring? :O I expect these should only escape the newline that comes after, but now that I think of it; we want those newlines as part of the docstring, like any normal Python docstring of 80 char max. Now I'm not entirely sure what to expect anymore. :S

Perhaps an even better reason to think of another way..

@Muream
Copy link
Contributor Author

Muream commented Jun 9, 2021

Wait, are you saying the \ gets included in the actual docstring? :O

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"

@mottosso
Copy link
Owner

mottosso commented Jun 9, 2021

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.

@Muream
Copy link
Contributor Author

Muream commented Jun 9, 2021

Sounds good, I'll go with that then 👍

@Muream
Copy link
Contributor Author

Muream commented Jun 9, 2021

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 arg0 in the stubs at the moment but they're on properties so I don't think it matters much. I'll have a look tomorrow if that's fixable. If not I think this is in a good state to be merged

@Muream
Copy link
Contributor Author

Muream commented Jun 9, 2021

Ah, will need to setup the release properly as well

@mottosso
Copy link
Owner

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?

@Muream
Copy link
Contributor Author

Muream commented Jun 10, 2021

I feel like this is going to make keeping consistent line length awkward because of the differences in the #define statement length
It does look less cluttered though.
I think I'm leaning more towards my solution but I don't have a super strong opinion there. As long as we're consistent, it's good with me.

@mottosso
Copy link
Owner

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.

@Muream
Copy link
Contributor Author

Muream commented Jun 10, 2021

Yeah I like that, best of both worlds 👍

@Muream Muream marked this pull request as ready for review June 10, 2021 12:03
@Muream
Copy link
Contributor Author

Muream commented Jun 10, 2021

@mottosso I think we're good now

again, hoping the release modification works without testing

@mottosso
Copy link
Owner

Let's go!

@mottosso mottosso merged commit b448f97 into mottosso:master Jun 10, 2021
@Muream Muream deleted the improve-stubs branch June 29, 2021 11:49
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

2 participants