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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync mypy_primer_comment with typeshed #12125

Merged
merged 1 commit into from
Feb 6, 2022
Merged

Sync mypy_primer_comment with typeshed #12125

merged 1 commit into from
Feb 6, 2022

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Feb 4, 2022

This PR adds a useful feature: now empty primer output will still add a comment: "According to mypy_primer, this change has no effect on the checked open source code."

Why is it useful?
Because in some cases it is not clear: is mypy_primer happy? Or is it broken?
For example, this PR #12118 does not have any comment.

I guess it would also benefit PR authors who can be sure that what they did is correct.

I will wait for mypy_primer to comment on this PR 馃檪

@sobolevn
Copy link
Member Author

sobolevn commented Feb 4, 2022

It looks like it still runs the old version: https://github.com/python/mypy/runs/5064684428?check_suite_focus=true

@JelleZijlstra
Copy link
Member

Yes, actions run based on what's on master, not on the PR.

I think this difference was intentional, perhaps because some mypy PRs (e.g., mypyc changes) don't affect the type checker and aren't expected to make a difference in mypy-primer. Then again, that's also true for some typeshed changes.

@sobolevn
Copy link
Member Author

sobolevn commented Feb 4, 2022

I guess if that's an issue we can add - files: spec to mypy_primer action.
So, mypyc only changes - will be ignored.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Yeah, the version of the action in the PR doesn't run to avoid security issues, since the action has write permissions on PRs.

I seem to remember some doubt about this being noisy, but I couldn't find where that was discussed and people on typeshed don't seem to mind, so let's go for it. Note that the real mypy-primer workflow already ignores mypyc, so we don't need to add files here.

The one change I would like is to minimise differences in the comment workflow between this and typeshed. E.g., I think typeshed acquired some nice change that uses a glob instead of manually listing the shards / the comment hider workflow runs in a different order due to not hiding on mypy. Let's update this PR to make the mypy version match typeshed as much as possible

@sobolevn
Copy link
Member Author

sobolevn commented Feb 5, 2022

I accidentally commited the requested changes to master: 26fdd45

Not quite used to the fact I can do that 馃槅
I hope it is fine, because the change in 26fdd45 is self-contained.

If needed, it can be reverted.

@sobolevn
Copy link
Member Author

sobolevn commented Feb 5, 2022

the comment hider workflow runs in a different order due to not hiding on mypy

I will address this in a separate PR, because I will need to investigate the changes there. They seem to be rather big 馃

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2022

I am going to merge it and test with my upcoming tuple PR 馃帀

@hauntsaninja
Copy link
Collaborator

Thanks, sounds good, I'm familiar with the comment hiding stuff, so can sync it myself :-)

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2022

@hauntsaninja I am working on right now 馃槉

matthewrv pushed a commit to matthewrv/mypy that referenced this pull request Feb 6, 2022
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.

None yet

3 participants