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

Replace mypy_integration_config repo with label flag #62

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Replace mypy_integration_config repo with label flag #62

merged 1 commit into from
Jun 10, 2022

Conversation

UebelAndre
Copy link
Contributor

Currently the mypy.ini config file is determined via a repository. This can lead to issues where one repository does not define this or two repositories attempt to create it using different config files. Instead of having users add the following in their workspace

load("//:config.bzl", "mypy_configuration")

mypy_configuration()

This PR has them update their .bazelrc files to pass a build flag which defines what config to use

build --@mypy_integration//:mypy_config=//:mypy.ini

In this case, a mypy.ini in the root of the current workspace would be used as the mypy config.

@ianoc
Copy link

ianoc commented May 27, 2022

Wouldn't a toolchain be the more usual way to configure a global setting like this, any reason to use a setting here instead?

@UebelAndre
Copy link
Contributor Author

Wouldn't a toolchain be the more usual way to configure a global setting like this, any reason to use a setting here instead?

These rules currently do not have a toolchain implemented. I think a toolchain would be fine but in the absence of all that plumbing, a global setting like this feels like the most effective step forward.

@ianoc
Copy link

ianoc commented May 27, 2022

Yeah fair enough, just seems like maybe a toolchain is the more expected manner than a setting was my only thought. But it does seem better. Sorry I was mostly just drive by curious why one would go with the setting, the overhead/boilerplate is a decent answer. ty!

@UebelAndre
Copy link
Contributor Author

@alexeagle do you have time sometime in the next week or so to review this?

@UebelAndre
Copy link
Contributor Author

@thundergolfer would you maybe be able to take a look? 🙏

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Looks like an improvement to me, thanks!

@alexeagle alexeagle merged commit ad1d482 into bazel-contrib:main Jun 10, 2022
@UebelAndre UebelAndre deleted the repo branch June 10, 2022 21:23
@UebelAndre
Copy link
Contributor Author

Looks like an improvement to me, thanks!

@alexeagle Thanks! Would it be possible to get a new release for this change?

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