https://pantsbuild.org/ logo
b

bitter-ability-32190

01/29/2022, 4:51 PM
I'm thinking folks will like this one: https://github.com/pantsbuild/pants/pull/14306 😄
❤️ 2
🙌 2
🚀 2
🏜️ 1
If people like it, I'll reset approvals and blast the codebase with the retrofit
h

hundreds-father-404

01/29/2022, 5:23 PM
This is awesome!
and blast the codebase with the retrofit
Let's save that for followup. Start with an easy-to-understand change w/ good tests and then probably still those 2 examples Makes it easier to understand Git history + revert things if we ever need.
b

bitter-ability-32190

01/29/2022, 5:23 PM
Fine with me, and yeah I should've said tests are a given 😁
(also commenting here because I'm on mobile) unfortunately there's no runtime access to the generic type (for good reason) so without some EXTRA magic, it's impossible But also that's a good thing as the property type isn't necessarily the flag type. (Same as why it's ok-ish you specify the attr name and repeat it as a flag name)
1
h

hundreds-father-404

01/29/2022, 5:27 PM
agreed, I don't think we should try inferring the type from MyPy type hint. Also that type hint is totally legal to leave off
But, it is a source of confusion imo. So I'm not 100% sold we shouldn't continue to require
type
as a kwarg -- seems probable, but better for that to be a dedicated change
b

bitter-ability-32190

01/29/2022, 5:36 PM
Very fair point
h

hundreds-father-404

01/29/2022, 5:36 PM
you got mail 😉 very excited about the idea of
BoolOption
and
StringOption
b

bitter-ability-32190

01/29/2022, 5:43 PM
Same
h

hundreds-father-404

01/29/2022, 8:20 PM
now that you've shown the light, I'm extra frustrated adding a new option the old way 🙈
b

bitter-ability-32190

01/29/2022, 8:53 PM
Lol yeah I got frustrated every time I had to rename the (now named) assets options 🤣
1
h

hundreds-father-404

01/30/2022, 2:37 AM
ready for me to merge?
b

bitter-ability-32190

01/30/2022, 9:10 AM
Oh yeah! And fwiw love your subclass idea.
This is gonna be fun
Nobody better steal my refactoring rights 🤣
😂 1
💯 1
h

hundreds-father-404

01/30/2022, 2:32 PM
Oh you know what, we limit
type=
to what is on here: https://www.pantsbuild.org/docs/rules-api-subsystems#type. There is less flexibility than the Target API We might want to consider renaming
Option
to
_Option
and only exposing
BoolOption
etc. Idea being to keep the API interface smaller while we figure out the best idioms for this all
b

bitter-ability-32190

01/30/2022, 4:51 PM
Let me try that as we refactor the code
(but probably still in separate PR)
Coming soon to a PR near you: concretely typed options! (Hopefully tomorrow)
❤️ 1
🙌 1