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.

Sign up for GitHub

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

Jump to bottom

feat: create transforms for adding dependencies #236

Merged
merged 1 commit into from
May 26, 2023
Merged

feat: create transforms for adding dependencies #236

merged 1 commit into from
May 26, 2023

Conversation

ahal
Copy link
Collaborator

@ahal ahal commented May 2, 2023

These transforms will implement part of the work that the multi_dep loader is doing in other projects.

Issue: #227

@ahal ahal self-assigned this May 2, 2023
@ahal
Copy link
Collaborator Author

ahal commented May 2, 2023

WIP, posting these now just to get them up for possible early feedback.

bhearsum
bhearsum reviewed May 2, 2023
View reviewed changes
Copy link
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is jam-packed with great functionality - I love it!

I'm a bit iffy on the name after_deps - it doesn't really tell me anything about what it does - but maybe that's just me. I'll throw out from_deps or tasks_from_deps as possible alternatives, but I'm also totally fine keeping the current name if you prefer.

docs/reference/transforms/after_deps.rst Outdated Show resolved Hide resolved
docs/reference/transforms/after_deps.rst Outdated Show resolved Hide resolved
@ahal
Copy link
Collaborator Author

ahal commented May 3, 2023

This is jam-packed with great functionality - I love it!

I'm a bit iffy on the name after_deps - it doesn't really tell me anything about what it does - but maybe that's just me. I'll throw out from_deps or tasks_from_deps as possible alternatives, but I'm also totally fine keeping the current name if you prefer.

Oh, I like from_deps! I'll go with that.

@ahal ahal mentioned this pull request May 4, 2023
Merged
4 tasks
@bendk bendk mentioned this pull request May 5, 2023
Merged
@ahal ahal force-pushed the after_deps branch 2 times, most recently from 7ee3c67 to 7ac8005 Compare May 25, 2023 20:35
@ahal ahal marked this pull request as ready for review May 25, 2023 20:36
@ahal ahal requested review from a team and bhearsum May 25, 2023 20:36
@ahal
Copy link
Collaborator Author

ahal commented May 25, 2023

I believe this is now ready. I've also created an example patch on VPN demonstrating how to use this:
https://github.com/mozilla-mobile/mozilla-vpn-client/compare/main...ahal:from_deps?expand=1

(note that multi_dep is still needed by other kinds in that repo, otherwise in practice there would be a lot more code removal :p)

@ahal
feat: create transforms for adding dependencies
9ab958b 
These transforms will implement part of the work that the multi_dep
loader is doing in other projects.

Issue: taskcluster#227
bhearsum
bhearsum approved these changes May 26, 2023
View reviewed changes
Copy link
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me!

@ahal ahal merged commit 9deb6b6 into taskcluster:main May 26, 2023
9 checks passed
@ahal ahal deleted the after_deps branch May 26, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhearsum bhearsum bhearsum approved these changes

Assignees

@ahal ahal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ahal @bhearsum