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

Revert #196 #203

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Revert #196 #203

merged 3 commits into from
Nov 27, 2023

Conversation

sizmailov
Copy link
Owner

@sizmailov sizmailov commented Nov 27, 2023

I made a few mistakes that led to this.

  1. I did a quick review, overlooking/ignoring suspicious changes
  2. I've ignored that feat: ✨ automatically replace invalid enum expressions with corresponding valid expression & import #196 effectively made the parser class 2-pass from 1-pass.

@ringohoffman Please don't take the revert personally.

@sizmailov
Copy link
Owner Author

Currently, stubgen does not provide a suitable interface to implement fixups on already parsed modules. Postponing everything to finalize() is working, but it's not the right tool for the job.

@sizmailov sizmailov changed the title Rever #196 Revert #196 Nov 27, 2023
@sizmailov sizmailov merged commit 99f14a1 into master Nov 27, 2023
16 checks passed
@sizmailov sizmailov deleted the revert-196 branch November 27, 2023 01:26
@ringohoffman
Copy link
Contributor

ringohoffman commented Nov 27, 2023

It's regrettable that there was a logic error. Are there no architectural changes that you would be happy with to support delayed InvalidExpression fixing?

My original implementation wasn't to do everything in finalize. If the concern is to raise an error when there is >1 possible definition for an enum field used as a default argument, we could just check if we ever try to register the same expression twice, and go back to performing the fixes before finalize.

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