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

Check *_pb2.pyi files again #10909

Merged
merged 8 commits into from
Oct 23, 2023
Merged

Check *_pb2.pyi files again #10909

merged 8 commits into from
Oct 23, 2023

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Oct 18, 2023

I was looking at Ruff vs Flake8 differences in typeshed and noticed this.

Start checking *_pb2.pyi files again as the issue seems to be resolved.
Fixes all NQA102 "# noqa: F821" has no matching violations that were ignored (we don't have a generation script for stubs/s2clientprotocol so that was done manually)

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 18, 2023

All these *_pb2.pyi files in our protobuf stubs are autogenerated with mypy-protobuf -- did you use the script here to regenerate them, or did you edit them manually? :)

If you used the latest version of mypy-protobuf to regenerate these files, we should probably update the pinned version here, so these changes aren't overridden the next time the files are regenerated:

MYPY_PROTOBUF_VERSION=v3.4.0

@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Oct 18, 2023

The script ends at the followign in git bash:

+ python3 -m venv venv
+ source venv/bin/activate
./scripts/generate_proto_stubs.sh: line 47: venv/bin/activate: No such file or directory

And immediately fails with the following in WSL:

/usr/bin/env: ‘bash\r’: No such file or directory

So I can't really validate locally

Edit: \r because the script is in CRLF, I'll open a PR to fix that.

@AlexWaygood
Copy link
Member

(Yeah, I've never run the script locally either)

@Avasam
Copy link
Collaborator Author

Avasam commented Oct 18, 2023

This should be better. I could auto-remove redundant F821 with Ruff once astral-sh/ruff#3011 is fixed
Completing #10914 resolved this issue.

@Avasam Avasam changed the title Check *_pb2.pyi files again and remove redundant suppressions Check *_pb2.pyi files again Oct 18, 2023
.flake8 Outdated Show resolved Hide resolved
@Avasam Avasam marked this pull request as draft October 19, 2023 21:30
@Avasam
Copy link
Collaborator Author

Avasam commented Oct 20, 2023

While we're at it:
Our Black configs "force-exclude" .*_pb2.pyi files with the following comment:

Exclude protobuf files because they have long line lengths
Ideally, we could configure Black to allow longer line lengths
for just these files, but doesn't seem possible yet.

But isn't that exactly what Black is for?
Did it use to make Black crash?
Is it because they're generated, so we don't want to bother formatting line-length? But then, shouldn't we not care about formatting them at all? Why is line-length specifically of concern here?

The protobuf stubs generation script even tries to run black (which currently ends up doing nothing), so I'm a bit confused.

@Avasam Avasam marked this pull request as ready for review October 23, 2023 00:08
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood merged commit e477c67 into python:main Oct 23, 2023
53 checks passed
@AlexWaygood
Copy link
Member

Thanks!

@Avasam Avasam deleted the flake8-_pb2.pyi branch October 23, 2023 00:25
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.

3 participants