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.

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 Target.has_field() as sugar for Target.has_fields() #9314

Merged
merged 1 commit into from
Mar 17, 2020

Conversation

Eric-Arellano
Copy link
Contributor

The main idiom we'll use for Target API code is:

relevant_tgts = [tgt for tgt in tgts if tgt.has_field(PythonSources)]
# do something with these tgts

has_field(PythonSources) is simpler to write than has_fields([PythonSources]). Because we will use this so often, this is nice sugar to add.

--

This also documents the 3 main methods exposed by Target.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks.

custom subclass `PythonSources`, both `python_tgt.has_field(PythonSources)` and
`python_tgt.has_field(Sources)` will return True.
"""
return self.has_fields([field])
Copy link
Member

Choose a reason for hiding this comment

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

It might be a micro optimization, but this method could probably have its own implementation to avoid the extra collection classes that are created here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I looked at factoring that out and it wasn't very pretty. I think this is likely a micro optimization.

We'll probably want to profile the Target API once the dust settles, and maybe this ends up making a difference to change down the road.

@Eric-Arellano Eric-Arellano merged commit 3c3aaed into pantsbuild:master Mar 17, 2020
@Eric-Arellano Eric-Arellano deleted the has-field branch March 17, 2020 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants