[stubgenc] fixes and typos #9877
Conversation
45b57d2 to
535fc74
Compare
|
Thanks for the PR and your attention to detail! It's okay to have all of these changes in a single PR, since they are all pretty simple. Having tests against a pybind module could be nice, assuming no extra big dependencies are needed and these tests aren't complex or slow. I'd prefer if the dependency could be included in |
535fc74 to
ebd8d01
Compare
There was a problem hiding this comment.
I've added basic check for generated stubs for real pybind11 project.
The check is full-text match. This is probably imperfect but I don't immediately see what alternative we have.
The C++ code is included as external dependency to a specific branch to my own repo. This is obviously cannot be merged as is. Please tell how to deal with it. Setting up "official" repo seems to be overkill, so I'm inclined to full-text include of linked repo to mypy. Again, I don't have strong opinion here.
I've placed expected stubs under test-data/stubgen/. Seems fine to me, but could be wrong.
|
Hmm the test set up feels rather complex and I can see it becoming painful to maintain. Also it would be nice if we can avoid numpy as a dependency, since it's fairly big and might slow down CI. Here are some alternatives (which aren't perfect though). Let me know what you think about them.
For option 2, you can pin a well-known, pre-existing package that uses pybind11 (so that the dep is already available and can be expected to stay relatively up-to-date). You could also set up a new PyPI package dedicated to this test only. Some mypy core devs would probably need write access to upload new versions of the package, and the build and upload scripts could perhaps live in the mypy repository. (I'm not very familiar with pybind11 so it's possible that my suggestions don't make much sense.) |
|
I think I would go with option 2 since some tests are better than none, and automatic test are better than manual. I've contacted pybind11 maintainers in gitter chat and I think there is a good chance to have a maintainable repo dedicated to test stubgen's ability to precisely render pybind11 modules. The only remaining question is where this project should be hosted (e.g. under This might take some time to compose a concise "pybind11 mypy demo" repo along with PyPI package, but it's surely worth it. (My test repo with occasional constructions is poorly structured and I think needs complete rewrite from scratch to serve as demo in any sense) cc: @henryiii, @YannickJadoul |
933480e to
f5976d1
Compare
|
I've moved the CI stubgenc test to a separate workflow restricted to changes in stubgenc-related files, so it won't run for most of the time. The workflow runs The demo project is updated to be shorter and more structured than what I had before. I think it's already good to be shipped as version @henryiii How do you feel about EDIT: CI calls bash script now |
b14a3cf to
d7e7a1e
Compare
|
When this would be moved under |
d7e7a1e to
dd87a10
Compare
|
I've released At this point I see this PR as complete. I've removed intermediate commits and rebased on current master (which already fixed one the typos from PR :-) |
dd87a10 to
c975b75
Compare
c975b75 to
d8f365a
Compare
|
Hi! Is there is anything left that blocks the PR? Do we wait for rights share/transfer, etc.? It's fine if it's just lack of time :) No rush |
Description
This PR consist of series of minor changes to
stubgenc, including typos in comment and (unfortunately) in code, one bug fix in docstring parsing and minor tweak to generate a better stubs for pybind11 modules. Hopefully commit messages are explanatory enough.This might be vague PR of small but rather disconnected changes, let me know if this must be split in smaller once.
Test Plan
So far this PR does not include test, therefore is marked as draft.
Locally I've additionally tested stubs generation for a small pybind11-based project (https://github.com/sizmailov/pybind11-project-example). As far as I can tell currently there is no tests in CI against actual pybind module. Not sure if it's desirable to have one.
Please let me know if tests for this changes are required.