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

Use default when None given to tag_class attribute #23547

Conversation

Silic0nS0ldier
Copy link
Contributor

With this, code like the following is valid.

pip = use_extension("@rules_python//python/extensions:pip.bzl", "pip")
pip.parse(
    hub_name = "bazel_pip_dev_deps",
    python_version = "3.8",
    requirements_lock = "//:requirements.txt",
    parallel_download = None,
)

Like with rules and repository_rules, the default (True in the example) will be applied.

@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Sep 8, 2024
@meteorcloudy
Copy link
Member

Wouldn't this be a problem is the user actually wants to pass None to the attribute?

@meteorcloudy
Copy link
Member

Like with rules and repository_rules, the default (True in the example) will be applied.

I see, this is consistent with rules and repository_rules. Then LGTM!

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 9, 2024
@Silic0nS0ldier
Copy link
Contributor Author

Wouldn't this be a problem is the user actually wants to pass None to the attribute?

As noted, this is for consistency. #23548 also makes None a possible input, in a round-about way (None usable as default). Allowing None to be truly passed through seems like more trouble than it'd be worth, with reasoning given on the aforementioned PR.

@fmeum
Copy link
Collaborator

fmeum commented Sep 9, 2024

@Silic0nS0ldier Apart from consistency, what's the use case you have in mind for this? In the example above, both removing the argument and setting it to True explicitly seem cleaner to me.

For build rules and repository rules, being able to use None to force a default value is important because it simplifies macros wrapping such rules. But that's not relevant for module extension tags, which don't support macros.

@Wyverald Wyverald added awaiting-user-response Awaiting a response from the author and removed awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally labels Sep 10, 2024
@Wyverald
Copy link
Member

I have the same question as @fmeum. It's unclear to me what this is useful for.

@Silic0nS0ldier
Copy link
Contributor Author

No specific use case in mind here, this was more of a knee-jerk response to noticing what is on the surface divergent behaviour for the attr module.

As it stands right now, there is no need to support None attributes for tags in MODULE.bazel.

I do think that this divergence should be documented. Perhaps in tag_class and attr docs. I'll close this and raise a PR that does just that.

Silic0nS0ldier added a commit to Silic0nS0ldier/bazel that referenced this pull request Sep 11, 2024
copybara-service bot pushed a commit that referenced this pull request Sep 18, 2024
Relates to #23547

`tag_class` is unique in a few ways to other functions that accept an attribute spec from the `attr` module.
1. It does not return a callable, but rather value containing metadata for `module_extension.tag_classes`.
2. The defined attributes outright reject `None` as a possible value, unlike `rule`, `aspect` and `repository_rule` which all fallback to the default as though no value were provided at all.

Supporting `None` makes sense for the other callable results as constructs such as `**kwargs` and functions/macros with default `None` arguments often expose them to such values. Allowing `None` to implicitly mean "use default" is useful in those contexts. This is not the case for `tag
_class`, which are interacted with in the extremely restricted context of `MODULE.bazel`.

This PR seeks to document how `None` is treated by declared attributes now that behaviour is (intentionally) inconsistent.

I also add links from builtin types such as `tag_class` to their factory function (`tag_class()`).

Closes #23595.

PiperOrigin-RevId: 676066782
Change-Id: Iacc7ca8b7b1560b2b79b7919ceb243504ff0e3cb
iancha1992 pushed a commit that referenced this pull request Sep 20, 2024
Relates to #23547

`tag_class` is unique in a few ways to other functions that accept an attribute spec from the `attr` module.
1. It does not return a callable, but rather value containing metadata for `module_extension.tag_classes`.
2. The defined attributes outright reject `None` as a possible value, unlike `rule`, `aspect` and `repository_rule` which all fallback to the default as though no value were provided at all.

Supporting `None` makes sense for the other callable results as constructs such as `**kwargs` and functions/macros with default `None` arguments often expose them to such values. Allowing `None` to implicitly mean "use default" is useful in those contexts. This is not the case for `tag
_class`, which are interacted with in the extremely restricted context of `MODULE.bazel`.

This PR seeks to document how `None` is treated by declared attributes now that behaviour is (intentionally) inconsistent.

I also add links from builtin types such as `tag_class` to their factory function (`tag_class()`).

Closes #23595.

PiperOrigin-RevId: 676066782
Change-Id: Iacc7ca8b7b1560b2b79b7919ceb243504ff0e3cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-user-response Awaiting a response from the author team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants