Default values of fields and un-parametrized consumers not accounted for during parametrization defaulting
#16175
Comments
stuhood
added a commit
that referenced
this issue
Jul 14, 2022
…rize`d targets for AsyncFieldMixin (#16176) Although the tests for #14519 were reasonably good, they tested with `StringField`, rather than additionally with `AsyncFieldMixin`. The latter changes the definition of equality for the `Field` to include its address, meaning that comparing the `Field` instance itself will never match for `Field`s from different targets. That made #14519... not particularly useful in production. This change fixes `Field` value comparisons to use `field.value`, references the newly opened #16175 (which is out of scope for now, given that it will likely be impacted by `__defaults__` and would be much harder to cherry-pick), and adds an additional test. Fixes #14519. [ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
to stuhood/pants
that referenced
this issue
Jul 14, 2022
…rize`d targets for AsyncFieldMixin (pantsbuild#16176) Although the tests for pantsbuild#14519 were reasonably good, they tested with `StringField`, rather than additionally with `AsyncFieldMixin`. The latter changes the definition of equality for the `Field` to include its address, meaning that comparing the `Field` instance itself will never match for `Field`s from different targets. That made pantsbuild#14519... not particularly useful in production. This change fixes `Field` value comparisons to use `field.value`, references the newly opened pantsbuild#16175 (which is out of scope for now, given that it will likely be impacted by `__defaults__` and would be much harder to cherry-pick), and adds an additional test. Fixes pantsbuild#14519. [ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
to stuhood/pants
that referenced
this issue
Jul 14, 2022
…rize`d targets for AsyncFieldMixin (pantsbuild#16176) Although the tests for pantsbuild#14519 were reasonably good, they tested with `StringField`, rather than additionally with `AsyncFieldMixin`. The latter changes the definition of equality for the `Field` to include its address, meaning that comparing the `Field` instance itself will never match for `Field`s from different targets. That made pantsbuild#14519... not particularly useful in production. This change fixes `Field` value comparisons to use `field.value`, references the newly opened pantsbuild#16175 (which is out of scope for now, given that it will likely be impacted by `__defaults__` and would be much harder to cherry-pick), and adds an additional test. Fixes pantsbuild#14519. [ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
that referenced
this issue
Jul 14, 2022
…rize`d targets for AsyncFieldMixin (Cherry-pick of #16176) (#16180) Although the tests for #14519 were reasonably good, they tested with `StringField`, rather than additionally with `AsyncFieldMixin`. The latter changes the definition of equality for the `Field` to include its address, meaning that comparing the `Field` instance itself will never match for `Field`s from different targets. That made #14519... not particularly useful in production. This change fixes `Field` value comparisons to use `field.value`, references the newly opened #16175 (which is out of scope for now, given that it will likely be impacted by `__defaults__` and would be much harder to cherry-pick), and adds an additional test. Fixes #14519. [ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
that referenced
this issue
Jul 14, 2022
…rize`d targets for AsyncFieldMixin (Cherry-pick of #16176) (#16179) Although the tests for #14519 were reasonably good, they tested with `StringField`, rather than additionally with `AsyncFieldMixin`. The latter changes the definition of equality for the `Field` to include its address, meaning that comparing the `Field` instance itself will never match for `Field`s from different targets. That made #14519... not particularly useful in production. This change fixes `Field` value comparisons to use `field.value`, references the newly opened #16175 (which is out of scope for now, given that it will likely be impacted by `__defaults__` and would be much harder to cherry-pick), and adds an additional test. Fixes #14519. [ci skip-rust] [ci skip-build-wheels]
|
In the case of many consuming targets using the default resolve, this could actually be quite annoying. We might need a solution for the 2.13 branch (unfortunately, before |
stuhood
changed the title
parametrized consumers not accounted for during parametrization defaulting
|
I don't really see a big risk with backporting the |
Eric-Arellano
pushed a commit
to Eric-Arellano/pants
that referenced
this issue
Jul 18, 2022
…r is `parametrize`d. (pantsbuild#16198) Addresses the second half of pantsbuild#16175 by filling address parameters for all consuming targets, rather than only those which are `parametrize`d themselves. [ci skip-rust] [ci skip-build-wheels]
Eric-Arellano
pushed a commit
to Eric-Arellano/pants
that referenced
this issue
Jul 18, 2022
…r is `parametrize`d. (pantsbuild#16198) Addresses the second half of pantsbuild#16175 by filling address parameters for all consuming targets, rather than only those which are `parametrize`d themselves. [ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
that referenced
this issue
Jul 18, 2022
As discussed on #16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see #12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And #12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes #16175. [ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
to stuhood/pants
that referenced
this issue
Jul 19, 2022
As discussed on pantsbuild#16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see pantsbuild#12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And pantsbuild#12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes pantsbuild#16175. [ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
to stuhood/pants
that referenced
this issue
Jul 19, 2022
As discussed on pantsbuild#16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see pantsbuild#12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And pantsbuild#12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes pantsbuild#16175. [ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
that referenced
this issue
Jul 19, 2022
) (#16220) As discussed on #16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see #12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And #12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes #16175. [ci skip-rust] [ci skip-build-wheels]
stuhood
added a commit
that referenced
this issue
Jul 19, 2022
) (#16219) As discussed on #16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see #12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And #12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes #16175. [ci skip-rust] [ci skip-build-wheels]
jyggen
pushed a commit
to jyggen/pants
that referenced
this issue
Jul 27, 2022
…r is `parametrize`d. (pantsbuild#16198) Addresses the second half of pantsbuild#16175 by filling address parameters for all consuming targets, rather than only those which are `parametrize`d themselves. [ci skip-rust] [ci skip-build-wheels]
jyggen
pushed a commit
to jyggen/pants
that referenced
this issue
Jul 27, 2022
As discussed on pantsbuild#16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see pantsbuild#12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And pantsbuild#12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes pantsbuild#16175. [ci skip-rust] [ci skip-build-wheels]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
#14519 can match the literal values of
Fields, but isn't aware of the default values of the fields. A common context where this is relevant is forresolvefields, which usually have a computed default value for the repo (at least until__defaults__are widely deployed and more of these legacy defaults are deprecated).Additionally, #14519 only triggers when a dependee is
parametrized. This means that the common case of lots of "unparametrized targets with the default resolve" ends up needing to explicitly fill all addresses.While possible to do (since the consumer is not parametrized, and so could explicitly spell out the resolve to use in its
dependencieslist), this imposes far too much boilerplate for someone introducing a new resolve in a repository.The text was updated successfully, but these errors were encountered: