-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: more featureful set-name for from-deps #484
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you feel about implementing the ability to specify registered name functions similar to how we handle group-by
? I worry that we aren't going to find a sane set of options here that are going to work for every conceivable case.
I'm picturing we would change set-name
to take a string that references a registered function (see group-by
). The default
function would do the current behaviour and we could provide another built-in function that doesn't strip out the kind.
Another option I like is removing set-name
entirely from from_deps
, and then creating a builtin taskgraph.transforms.set_name
transform that can be run afterwards.
Note that I have a couple PRs that break backward compat teed up already, so if we time this right we don't have to worry about making a major version bump just for this.
I'm open to other ideas here, yeah. Of the two you've mentioned I'm partial to being able to provide a function. I can probably whip that up pretty quickly. |
5c40a0f
to
bbb250e
Compare
@ahal - the latest needs docs, comment, etc. changes still - but can you have a quick look when you have a moment to see if it's what you had in mind? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks just like I was thinking!
bbb250e
to
8a4655d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This is useful in cases where many tasks are only distinguished by a common suffix, and have a common task downstream of them.
As a concrete example, in Translations, tasks are named after their kind + the language pair they're running against, and I'm working on getting beetmover running against all of them. At the moment,
from-deps
generate names likebeetmover-ru-en
for every single task, causing collisions.Note that we already do this sort of naming for things like beetmover and signing tasks in Firefox, where we end up with task names like
beetmover-repackage-macosx64-shippable/opt
andshippable-l10n-mac-signing-macosx64-shippable-17/opt
. (We don't usefrom-deps
there, but this or similar support is likely to be needed to every move to it.)