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

Ban * globs and ! ignores in source: str field #13629

Merged
merged 4 commits into from
Nov 16, 2021

Conversation

Eric-Arellano
Copy link
Contributor

There's no reason to support them. The SingleSourceField should be 0-1 file, so a glob would cause an error. While it is technically possible to have a glob that only matches one file, that's error-prone.

Enforcing this unblocks us from rendering a useful file name representing the target type in more places without needing to hydrate. For example, our streaming workunit handler code uses Address.filename, but that doesn't work with manually created per-file targets like python_source: #13627. This PR means that we can instead use SingleSourceField.file_path for the same effect.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@@ -16,7 +16,7 @@
from pants.core.util_rules.external_tool import DownloadedExternalTool, ExternalToolRequest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes to this file aren't strictly coupled and I can revert them if you'd like. But I noticed it while working on this PR

src/python/pants/engine/target.py Outdated Show resolved Hide resolved
src/python/pants/engine/target.py Outdated Show resolved Hide resolved
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano merged commit 7f81a1e into pantsbuild:main Nov 16, 2021
@Eric-Arellano Eric-Arellano deleted the ban-globs branch November 16, 2021 03:13
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Nov 16, 2021
…antsbuild#13629)

There's no reason to support them. The `SingleSourceField` should be 0-1 file, so a glob would cause an error. While it is technically possible to have a glob that only matches one file, that's error-prone.

Enforcing this unblocks us from rendering a useful file name representing the target type in more places without needing to hydrate. For example, our streaming workunit handler code uses `Address.filename`, but that doesn't work with manually created per-file targets like `python_source`: pantsbuild#13627. This PR means that we can instead use `SingleSourceField.file_path` for the same effect.

[ci skip-rust]
[ci skip-build-wheels]

# Conflicts:
#	src/python/pants/engine/target_test.py
Eric-Arellano added a commit that referenced this pull request Nov 16, 2021
…13629) (#13636)

There's no reason to support them. The `SingleSourceField` should be 0-1 file, so a glob would cause an error. While it is technically possible to have a glob that only matches one file, that's error-prone.

Enforcing this unblocks us from rendering a useful file name representing the target type in more places without needing to hydrate. For example, our streaming workunit handler code uses `Address.filename`, but that doesn't work with manually created per-file targets like `python_source`: #13627. This PR means that we can instead use `SingleSourceField.file_path` for the same effect.

[ci skip-rust]
[ci skip-build-wheels]
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