Skip to content

Post stubtest results to GitHub checks #25586

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.

Sign up for GitHub

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

Jump to bottom
Merged
ksunden merged 1 commit into from
Apr 17, 2023
Merged

Post stubtest results to GitHub checks #25586

ksunden merged 1 commit into from
Apr 17, 2023

Conversation

@QuLogic
Copy link
Member

@QuLogic QuLogic commented Mar 31, 2023

PR Summary

Unfortunately, the way that stubtest outputs its error messages makes it difficult to post a message on both the .py and .pyi stub, so we decided to take the full message and post it only on one.

As a test, I have made PR in my repo QuLogic#30 which started from the previously-unfixed #25542, and then I also added a commit with multiple argument mismatches to confirm that stubtest didn't output some different format.

Results are at the bottom of https://github.com/QuLogic/matplotlib/pull/30/files

PR Checklist

Documentation and Tests

  • [n/a] Has pytest style unit tests (and pytest passes)
  • [n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [n/a] New plotting related features are documented with examples.

Release Notes

  • [n/a] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • [n/a] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • [n/a] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@QuLogic QuLogic added topic: typing CI: testing CI configuration and testing labels Mar 31, 2023
@QuLogic QuLogic added this to the v3.8.0 milestone Mar 31, 2023
@ksunden
Copy link
Member

ksunden commented Mar 31, 2023 •
edited
Loading

The case that may need to be looked at a bit is the missing stub case, as that does not have a stub path to parse.

Edit: on second thought, it may give the path of the parent object or just line 0 of the module, but still worth a look

@QuLogic
Copy link
Member Author

QuLogic commented Mar 31, 2023

Would that be for a new Python file entirely?

@ksunden
Copy link
Member

ksunden commented Mar 31, 2023

No, just for a new method is what I was thinking

@QuLogic
Copy link
Member Author

QuLogic commented Apr 1, 2023

The stubtest output for that case doesn't include a line number, so it doesn't get posted, but the other errors do work fine. I'm not sure if I can get it to fall back to posting on the .py or if it would confuse it.

@QuLogic
Copy link
Member Author

QuLogic commented Apr 1, 2023

I also just added an extra function in the stub, but not the Python file, and I see that that doesn't get posted either; it looks like there's a slightly difference in output in that case, which I should be able to cover.

@QuLogic QuLogic force-pushed the reviewdog-mypy-stubtest branch from eff69d5 to 2d05200 Compare April 6, 2023 08:06
@QuLogic
Copy link
Member Author

QuLogic commented Apr 6, 2023

OK, this is now able to post 1) differences between stubs and runtime; 2) items in stubs but not in runtime. Unfortunately, the case with items in runtime but not stubs cannot be caught, AFAICT.

See https://github.com/QuLogic/matplotlib/pull/30/files

@QuLogic QuLogic mentioned this pull request Apr 6, 2023
Open
@QuLogic
Copy link
Member Author

QuLogic commented Apr 6, 2023

I opened an issue on stubtest to see if concise error messages could contain a bit more information (linked above). This could simplify this posting process as message would not be multiline.

oscargus
oscargus approved these changes Apr 15, 2023
View reviewed changes
Copy link
Member

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

Let's try this and see what happens.

@ksunden ksunden merged commit eb3fa7e into matplotlib:main Apr 17, 2023
@QuLogic QuLogic deleted the reviewdog-mypy-stubtest branch April 17, 2023 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ksunden ksunden ksunden approved these changes

@oscargus oscargus oscargus approved these changes

Assignees

No one assigned

Labels

CI: testing CI configuration and testing topic: typing

Projects

None yet

Milestone

v3.8.0

Development

Successfully merging this pull request may close these issues.

3 participants

@QuLogic @ksunden @oscargus