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

Correctly describe the origin of invalid specs, e.g. --paths-from #15730

Merged

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Jun 2, 2022

Before:

❯ ./pants paths --from=fake/f.py --to=src/python/pants/util/strutil.py
12:54:23.99 [ERROR] 1 Exception encountered:

  Exception: Unmatched glob from CLI arguments

After:

❯ ./pants paths --from=fake/f.py --to=src/python/pants/util/strutil.py
12:54:23.99 [ERROR] 1 Exception encountered:

  Exception: Unmatched glob from the option `--paths-from`: "fake/f.py"

It was a lie to hardcode "CLI arguments".

Most of these rules should never error because we only error if the directory of the CLI specs do not exist, and we programmatically compute the specs based on already existing targets. But still, this gives us much better debugging if those assumptions are violated.

This also unblocks us from #14468. The Specs will be able to pass down their description_of_origin to AddressInput for much better errors with invalid AddressLiteralSpecs.

[ci skip-build-wheels]

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

[ci skip-rust]
@@ -166,7 +166,9 @@ class _ParseOneBSPMappingRequest:
async def parse_one_bsp_mapping(request: _ParseOneBSPMappingRequest) -> BSPBuildTargetInternal:
specs_parser = SpecsParser()
specs = specs_parser.parse_specs(
request.definition.addresses, convert_dir_literal_to_address_literal=False
request.definition.addresses,
description_of_origin=f"the BSP mapping {request.name}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tdyas is this an accurate description?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, looks accurate

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Sweet stuff.

# 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]
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.

Great: thanks!

I can't reproduce the failing test that caused it. Ran the tests 10 times.

# 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 enabled auto-merge (squash) June 2, 2022 22:16
@Eric-Arellano Eric-Arellano merged commit 4171322 into pantsbuild:main Jun 3, 2022
@Eric-Arellano Eric-Arellano deleted the specs-description-of-origin branch June 3, 2022 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants