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

Allow for generated files #11

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dmadisetti
Copy link
Contributor

Sorry I'm back

Allows for generated files to be checked by mypy, and for external generated files to be ignored.

I've had this patch since my last PR, but only just got round to writing some tests. This might sound super edge-case and useless, but arose from me implementing this project with protobuf

If for some reason, a generated python file is meant to be explicitly checked, then we handle that case too.

@thundergolfer
Copy link
Collaborator

Sorry I'm back

Haha don't apologise! It's awesome to have contributors.

@thundergolfer
Copy link
Collaborator

This might sound super edge-case and useless, but arose from me implementing this project with protobuf

Can you expand on this? You were trying to implement this integration's functionality with protobuf? I don't quite understand.

@dmadisetti
Copy link
Contributor Author

dmadisetti commented Feb 26, 2020

This might sound super edge-case and useless, but arose from me implementing this project with protobuf

Can you expand on this? You were trying to implement this integration's functionality with protobuf? I don't quite understand.

Yeah! So I'm just trying to type-check some code that implements protobuf, which generates python for data serialization. My BUILD looks something like:

load(
    "@build_stack_rules_proto//python:python_proto_library.bzl",
    "py_proto_library",
)
py_proto_library(
    name = "py_proto",
    deps = ["//configs:proto"],
)
py_test(
    name = "pd3_test",
    srcs = ["pd3_test.py"],
    deps = [
        ":py_proto",
        "//pd3",
    ],
) 

Without the added changes, the invoked mypy command is:

external/mypy_integration/mypy/mypy --verbose --bazel --package-root .  --config-file external/mypy_integration_config/mypy.ini --cache-map bazel-out/k8-fastbuild/bin/configs/config_pb2.py bazel-out/k8-fastbuild/bin/configs/config_pb2.py.meta.json bazel-out/k8-fastbuild/bin/configs/config_pb2.py    .data.json python/pd3.py python/pd3.py.meta.json python/pd3.py.data.json python/utils.py python/utils.py.meta.json python/utils.py.data.json python/pd3_test.py python/pd3    _test.py.meta.json python/pd3_test.py.data.json -- bazel-out/k8-fastbuild/bin/configs/config_pb2.py python/pd3.py python/utils.py python/pd3_test.py

and breaks with mypy: can't read file 'bazel-out/k8-fastbuild/bin/configs/config_pb2.py': No such file or directory. I think it's fair that generated files from a dependency should not be explicitly typed (same motivation for excluding external sources), so we can just remove it from the command. A clear indication that this file was generated is the fact the path is under bazel-out/k8-fastbuild/bin/ (or darwin-fastbuild for macs, who knows for windows) not relative to the files, so we just filter for that.

However, if for some reason the generated file is included in srcs, it should be typed.

There may be a better way, but this seems reasonable-ish? I could play around with the examples folder if I get time

@dmadisetti
Copy link
Contributor Author

🤔 This is starting to look more like a bit of a hack. Let me know if you have other ideas. Fairly certain that bazel uses "/" on windows as well.

@thundergolfer
Copy link
Collaborator

However, if for some reason the generated file is included in srcs, it should be typed.

What could that reason be? If it's generated code, it should never be included in the srcs attr of a py_* rule right?

@dmadisetti
Copy link
Contributor Author

The tests I provided do exactly this: https://github.com/thundergolfer/bazel-mypy-integration/pull/11/files#diff-c40927b6657800717d4b0b498e1bad55R53

Can vs should is an important distinction though. I'd agree it's not good practice, but mypy shouldn't shouldn't break inexplicably.

@dmadisetti
Copy link
Contributor Author

Additional changes are for compat with thundergolfer/bazel-python-mypy-protobuf#2

Tests pass and the project builds

@ashwin153
Copy link

@thundergolfer Any reason why this can't be merged? The integration is currently unusable in repositories that use Protocol Buffers / gRPC.

@dmadisetti
Copy link
Contributor Author

If I resolve do you think we can get this in? About 3 years now, and I just ran into this problem again.

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