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

Fix test-stubgen.sh #16500

Conversation

bluenote10
Copy link
Contributor

As mentioned in #16306 (comment) it looks like the CI for the stub generator is no longer working.

This PR is just an experiment trying to reactivate it. I'd assume the CI now to fail, because at least locally the stub generator doesn't produce what the expected output is.

@bluenote10
Copy link
Contributor Author

bluenote10 commented Nov 15, 2023

This experiment seems to confirm that --include-docstrings does not work as expected.

@chadrik I'd assume that at some point during your work on #13284 it must have been working, because you will most likely not have written these output files by hand 😉. Most likely during the final phase of wrapping up the PR something happened that broke --include-docstrings but that got hidden by the bug in the test script. Do you have a idea what that could have been to fix it?

@chadrik
Copy link
Contributor

chadrik commented Nov 15, 2023

Thanks for all your help identifying and pushing this along, @bluenote10. I already got started on an PR with your suggested fix as well.

Most likely during the final phase of wrapping up the PR something happened that broke --include-docstrings but that got hidden by the bug in the test script. Do you have a idea what that could have been to fix it?

My PR completely restructured stubgen into an object-oriented design, so it changed a lot of things. I ran into numerous nasty merge conflicts during the months that it sat in review, and the last one was with --include-docstrings. The code for handling include_docstings on the C-extension side probably went away completely, because there was almost no code reuse between the pure-python and C-extension code paths prior to my change. I rebased it, fixed the issues, and saw the tests were passing and thought I was done. With these tests now showing the problems, it shouldn't be too hard to fix.

@bluenote10
Copy link
Contributor Author

With these tests now showing the problems, it shouldn't be too hard to fix.

Great, thanks for looking into it! Feel free to ping me for reviewing / experimenting if needed.

I'll close this one then, because it was just for the experiment.

@bluenote10 bluenote10 closed this Nov 15, 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