-
Notifications
You must be signed in to change notification settings - Fork 81
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
Generate the Python stubs automatically #1661
Generate the Python stubs automatically #1661
Conversation
44284b9
to
c4766ec
Compare
The generated signature for |
I think |
c4766ec
to
e76a5a8
Compare
Type annotations are now supported by the stub generator. I've introduced a new I've also brought all of the C++-defined docstrings up-to-date with the manual changes and type annotations in the stubs. This should resolve all of the spurious changes. This PR is basically finished now. I'll un-draft it once #1662 is merged and I've rebased this PR onto that (so that this PR's diff is more readable for reviewing). |
Not quite finished yet, but it can generate a Plasma.py that's relatively close to what we currently have in the repo.
7e9ba51
to
a5e144f
Compare
@@ -54,11 +63,11 @@ class PtGameJoinError: | |||
kGameJoinErrAlreadyJoined = 6 | |||
kGameJoinErrNoInvite = 7 | |||
kNumGameJoinErrors = 8 | |||
|
|||
kGameJoinPending = 4294967295 |
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.
Previously this was -1, but I'm guessing it's handled as a uint internally so it keeps the positive value?
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.
I guess a good question is whether a comparison against -1 would ever work in Python, maybe this MAX_UINT32 is the only correct option
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.
Yeah, it's declared in C++ as kGameJoinPending = (uint32_t)-1
, so it gets output as unsigned.
Also worth noting that I used a 64-bit client to generate the stubs. Plasma's custom EnumValue
s store the numeric value as Py_ssize_t
, so it's possible that a 32-bit client would see this constant as -1
on the Python side. That seems like something we should fix...
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.
I wonder if would be clearer to use 0xFFFFFFFF
?
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.
Hmm, how would we detect this in the generator though? Add a special case for 0xffffffff
, or just use hex for all values larger than some number?
In any case, I've now regenerated the stubs using a 32-bit client, so we can ignore this until the underlying problem with kGameJoinPending
is fixed.
@@ -54,11 +63,11 @@ class PtGameJoinError: | |||
kGameJoinErrAlreadyJoined = 6 | |||
kGameJoinErrNoInvite = 7 | |||
kNumGameJoinErrors = 8 | |||
|
|||
kGameJoinPending = 4294967295 |
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.
I wonder if would be clearer to use 0xFFFFFFFF
?
def add_indents(indent: str, lines: Iterable[str]) -> Iterable[str]: | ||
for line in lines: | ||
if line: | ||
yield indent + line | ||
else: | ||
# Don't add extra whitespace on blank lines. | ||
yield "" |
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.
This could be replaced with textwrap.indent()
.
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.
This is a good idea, but I would prefer to do this in a later PR, because it changes the formatting of generated docstrings that contain newlines. It turns out we have quite a few of those and it would clutter up the diff with even more whitespace changes.
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.
Interesting. I thought based on the documentation that it would give the same result, especially if we provided a predicate, eg
add_indents = functools.partial(textwrap.indent, predicate=lambda line: bool(line))
(The call to bool()
is for the sake of clarity and could be omitted.)
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.
With that being said, with the need for functools.partial()
, we might be reaching too far into the bag of tricks. I think I'd be okay with keeping add_indents()
if we remove the string concat in favor of an f-string 😉
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.
The "problem" is that textwrap.indent
will indent every line, whereas my add_indents
lets you insert newlines that will not cause an indent, e. g. add_indents(" ", ["indented\nnot indented", "indented again"])
.
This behavior was by accident, but it turned out to be necessary for matching Cyan's original formatting, where multiline docstrings only have their first line indented and not the following lines.
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.
Makes sense. Thanks for the clarification
Fixes CI errors because of the stub incorrectly indicating that these keyword arguments are required.
a5e144f
to
83c05f3
Compare
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.
(Can you tell that I'm not a fan of string concats?)
Co-Authored-By: Adam Johnson <[email protected]>
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.
This will make maintaining the stubs significantly easier. Thanks!
Closes #1660. Adds a script to automatically generate the Python "stub" files under Scripts/Python/plasma. The script must be run from the in-game Python console:
This will generate stubs for all Plasma built-in modules into a subfolder "plasma_stubs_generated" under the game folder.
Everything happens fully automatically using runtime introspection. Only one part is "semi-manual": because Python cannot introspect the signatures of C-defined functions, all function parameters must be specified manually in the C++-defined function docstrings, on a line starting with
Params:
. This convention was apparently used by Cyan originally, so most function docstrings already follow this format.The generator itself is almost completely finished, but this PR still needs a bit of work:
Params:
lines in the C++-defined docstrings.Returns:
.With those things in mind, early feedback is welcome. I'm particularly interested in the opinion of @Hoikas, who has recently hand-written some stubs that will be overwritten by this script.