Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add filtering subsystem to permit skipping targets by tags #7275
Add filtering subsystem to permit skipping targets by tags #7275
Changes from 4 commits
27d9b2c
2a4ca6a
8c174ce
496c1fd
0b37c8a
3e62727
b0117f1
04b6435
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This should probably be conditional on whether a
Task
actually opts-in to Target filtering: asget_targets
notes, very few tasks actually do, and for manyTasks
, target filtering doesn't make sense. Having this on "by default" like this will make it show up in the help and documentation for a whole bunch ofTasks
for which it will have no effect.As with @wisechengyi's initial patch, I would suggest a classmethod/classproperty of
Task
that subclasses could override to enable filtering (with filtering disabled by default), and then conditionally requesting the subsystem here if that property was True.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.
Good point! Yes, this will be a necessary follow-up.
But does this subsystem-based design address your v2 engine concerns?
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.
It does, thanks! This is very easy to transition to v2.
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.
👍 Addressing this in #7283
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.
FYI the reason this code has to wrap
filter()
withlist()
is that in Python 3 (and with the Future library's backports),filter()
returns a lazy generator.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.
I think you're already aware of this, but not many tasks use this method (they use
self.context.targets
) so there may need to be follow-up: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.
For context: This method was a relatively recent addition, to support lint/fmt tasks being optionally transitive or skipped entirely. So currently they are the only tasks that consult this method. However those tasks are also the immediate use case for this feature. Any other task we want to use it for will have to be modified to use this method, as you suggest, assuming we think that's worth doing in the v1 engine.