I m thinking folks will like this one <https github com pant Pants #development

Join Slack

I'm thinking folks will like this one: <https://gi...

# development

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

bitter-ability-32190

01/29/2022, 5:21 PM

If people like it, I'll reset approvals and blast the codebase with the retrofit

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.

bitter-ability-32190

01/29/2022, 5:23 PM

Fine with me, and yeah I should've said tests are a given 😁

bitter-ability-32190

01/29/2022, 5:26 PM

(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

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

hundreds-father-404

01/29/2022, 5:27 PM

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

bitter-ability-32190

01/29/2022, 5:36 PM

Very fair point

hundreds-father-404

01/29/2022, 5:36 PM

you got mail 😉 very excited about the idea of

BoolOption

and

StringOption

bitter-ability-32190

01/29/2022, 5:43 PM

Same

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 🙈

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

hundreds-father-404

01/30/2022, 2:37 AM

ready for me to merge?

bitter-ability-32190

01/30/2022, 9:10 AM

Oh yeah! And fwiw love your subclass idea.

bitter-ability-32190

01/30/2022, 9:10 AM

This is gonna be fun

bitter-ability-32190

01/30/2022, 9:11 AM

Nobody better steal my refactoring rights 🤣

😂 1

💯 1

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

_Option

and only exposing

BoolOption

etc. Idea being to keep the API interface smaller while we figure out the best idioms for this all

bitter-ability-32190

01/30/2022, 4:51 PM

Let me try that as we refactor the code

bitter-ability-32190

01/30/2022, 5:03 PM

(but probably still in separate PR)

bitter-ability-32190

02/01/2022, 10:32 PM

Coming soon to a PR near you: concretely typed options! (Hopefully tomorrow)

❤️ 1

🙌 1

bitter-ability-32190

02/02/2022, 8:44 PM

https://github.com/pantsbuild/pants/pull/14331

Open in Slack

Previous Next