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

Warnings when building documentation after type stubs changes #2666

Open
swt2c opened this issue Jan 9, 2025 · 4 comments · May be fixed by #2668
Open

Warnings when building documentation after type stubs changes #2666

swt2c opened this issue Jan 9, 2025 · 4 comments · May be fixed by #2668

Comments

@swt2c
Copy link
Collaborator

swt2c commented Jan 9, 2025

After the type stubs changes, when running build.py etg (without --no-doc), the process emits a bunch of warnings like the below:

SEVERE: Incompatibility between function/method signature and list of parameters in `wx.glcanvas.GLAttribsBase.AddAttribBits`:

The parameter `searchVal : int` appears in the method signature but could not be found in the parameter list.

  ==> Function/Method signature from `extractors`: AddAttribBits(searchVal : int, combineVal : int)
  ==> Parameter list from wxWidgets XML items:     ['searchVal', 'combineVal']

This may be a documentation bug in wxWidgets or a side-effect of removing the `wx` prefix from signatures.

@lojack5 any change you can address this one?

@infinity77
Copy link
Contributor

Back in time when the signature extractor was written, there was no such thing as annotations (I.e., the Perl-ification of Python), so there was no provision of extracting the parameter “names” as they are now.

the docstrings parsers will have to be modified to account for this. In particular, the CheckSignature method in ParameterList class in the sphinx_generator.py module has to be massaged a bit…

@lojack5
Copy link
Contributor

lojack5 commented Jan 9, 2025

I have no way to test this since I still can't figure out how to run without --nodoc (pretty sure it has something to do with not being able to find graphviz, even though I added it to PATH).

My quick guess is add:

arg = arg.split(':')[0].strip()

here:


Although I'm not 100% sure about the difference between arg and myarg here.

Anyway, I'm uncomfortable making a pull request for this myself since I can't test the results myself.

@lojack5 lojack5 mentioned this issue Jan 12, 2025
4 tasks
@lojack5
Copy link
Contributor

lojack5 commented Jan 12, 2025

So based on the docs build log with the mentioned change here: https://github.com/echoix/Phoenix/actions/runs/12729067512/job/35480417371

Looks like this clears up most of the issues, there's a few I saw that can further be fixed, and a few that we can't really do anything about if we want accurate type-stubs:

  1. Python reserved keywords as parameter names (I counted 6 occurrences):
  ==> Function/Method signature from `extractors`: __init__(_is : InputStream)
  ==> Parameter list from wxWidgets XML items:     ['is']
  ==> Function/Method signature from `extractors`: RefreshRowsColumns(_from : Position, to : Position)
  ==> Parameter list from wxWidgets XML items:     ['from', 'to']

These are all reserved Python keywords (I saw, is, from and def), so they cannot be identifiers. The type-stubs would error out with Syntax Errors if these were left as the original names.

About the only way to "fix" these would be adding more special case code in the sphinx generator to trick it by removing the leading _ from a name if it's a reserved python keyword. These weren't a problem before, since these were hidden by the old type stubs just having *args, **kwargs as their signature.

I will say - this probably should be addressed at some point (by renaming the C++ names of the variable to non-Python keywords), because attempting to pass values to these as keyword arguments won't work, regardless of what the type-stubs say:

wx.FileConfig(is=mystream) # SyntaxError, assuming we somehow got the typestub to be ok with `is` as a name
wx.FileConfig(_is=mystream) # ArgumentError most likely, because the type-stub says `_is` is the name, but SIP is expecting `is`.
  1. Overly aggressive C++ syntax removal (I counted 2 occurrences):
  ==> Function/Method signature from `extractors`: SatisfyConstraint(raints : LayoutConstraints, win : Window)
  ==> Parameter list from wxWidgets XML items:     ['constraints', 'win']

This should be fairly straightforward to fix. It's clearly caused by

for txt in ['const', '*', '&', ' ']:

Probably the best fix is to replace that with a regex that finds consts not in a larger word, something like: (const(?![\w\d]))

  1. Probably fairy fixable, I'm just not sure the best path for it yet (1 occurrence):
  ==> Function/Method signature from `extractors`: SetAssertMode(AppAssertMode : AppAssertMode)
  ==> Parameter list from wxWidgets XML items:     ['wxAppAssertMode']

It's clear what's happening here, wxAppAssertMode is a class/enum name in the C++ code, so the "clean wx prefix" part of the code is removing it to get at what it assumes is wanted, the class name AppAssertMode. Probably fixable by reworking the clean wx prefix code / usage to not run on what's identified as the argument name (though testing would definitely be required to make sure that doesn't break something else).

@lojack5 lojack5 linked a pull request Jan 12, 2025 that will close this issue
9 tasks
@Metallicow
Copy link
Contributor

(though testing would definitely be required to make sure that doesn't break something else).

I agree that this has not broken my concentration reviewing it for 3+ hours.
I agree that this can be that.
I agree LoJack has did a better than average submittion.
I agree the subject could get astronomically dumb by continuing.
I confirm there was NOT any Artificial Intelligence used in making my decision.
I agree everyone involved in the correction of the machines ways be added to the credits.
Thanks LoJack
, signing out
, your fellow contributors...

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 a pull request may close this issue.

4 participants