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

feat: ✨ automatically replace invalid enum expressions with corresponding valid expression & import #196

Merged
merged 10 commits into from
Nov 25, 2023

Conversation

ringohoffman
Copy link
Contributor

Inspired by: #192

I think this feature supersedes the need for --enum-class-locations, though I did not remove it here. At least that is my goal. I want this project to be as easy for people to pick up as possible, and this was something I thought should be possible for pybind11_stubgen to do on behalf of the user.

We tweak RewritePybind11EnumValueRepr to delay report_error() for enum expressions. We remember all of the invalid enum expressions we have seen as we crawl through the module. When we find the definition of a value corresponding to an enum repr, we use it to fix invalid expressions we have seen or will see. If there are still invalid enum expressions left over, we report_error() during finalize().

@sizmailov
Copy link
Owner

This should eliminate the need for enum location specification for most use cases.

This PR introduces a bit of guessing to stub generation if the module has two enums with the same class and value names. Depending on the order of traversing the module, the resulting stubs could be different.

I suggest the following changes:

  • Defer all the substitutions to the very end to catch such ambiguous replacements
  • Not replace ambiguous enum values, e.g. foo(x: Any = <Color.RED: 3>)
  • Replace unambiguous enums (e.g. foo(x: my_module.Car.Color = <Color.RED: 3>)
  • Do not import values into the module.

@ringohoffman
Copy link
Contributor Author

@sizmailov are you planning to drop support for 3.7 at some point? It reached end of life status 5 months ago. I would like it if we did, because I want to use the assignment expression operator (:=). I think it can be great for readability if used correctly.

3.8's EOL is coming in only 10 months, also. numpy has already dropped support for it in preparation.

What are your thoughts about this?

so that we can detect if an identical enum + field was defined somewhere else: sizmailov#196 (comment)
these were being created from a set, so the test output wasn't matching since the order is not deterministic
@sizmailov
Copy link
Owner

I think we should aim for the same versions as pybind11, which is currently 3.6+.
I don't remember exactly why I didn't support 3.6. Probably, it was the troubles of setting up the test for CI 😞

So Python 3.7 support would stay for quite a while, for better or worse.

@ringohoffman
Copy link
Contributor Author

ringohoffman commented Nov 25, 2023

@sizmailov Okay, I gave it my best effort.

  1. I moved the fixing to finalize().
  2. I only import module names.
  3. I check to see if there are multiple possible definitions for an invalid enum repr, and raise an error like:
pybind11_stubgen - [  ERROR] In finalize : Enum member '<ConsoleForegroundColor.Magenta: 35>' could not be resolved; multiple identical definitions found in: 'demo._bindings.duplicate_enum', 'demo._bindings.enum'
  1. I added a new test that creates an ambiguous enum using ConsoleForegroundColorDuplicate. I had to update --ignore-invalid-expressions to incldue <ConsoleForegroundColor\\.Magenta: 35>.
  2. I needed to add a LoggerData override of finalize() to create a new layer so that I could call report_error() from finalize().

Copy link
Owner

@sizmailov sizmailov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

The change looks good to me.

Just a couple of doc suggestions.

pybind11_stubgen/parser/errors.py Outdated Show resolved Hide resolved
pybind11_stubgen/parser/mixins/fix.py Outdated Show resolved Hide resolved
tests/demo.errors.stderr.txt Outdated Show resolved Hide resolved
@sizmailov
Copy link
Owner

If you don't mind, I'll publish it as 2.4.1

@ringohoffman
Copy link
Contributor Author

ringohoffman commented Nov 25, 2023

I don't mind! Thank you for reviewing my work!

@sizmailov sizmailov merged commit 731969b into sizmailov:master Nov 25, 2023
16 checks passed
@ringohoffman ringohoffman deleted the automatic-enum-fix branch November 25, 2023 10:45
sizmailov added a commit that referenced this pull request Nov 27, 2023
…orresponding valid expression & import (#196)"

This reverts commit 731969b.
sizmailov added a commit that referenced this pull request Nov 27, 2023
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.

2 participants