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

Remove BuildFile #10102

Merged
merged 5 commits into from Jun 20, 2020
Merged

Conversation

Eric-Arellano
Copy link
Contributor

We are now able to remove our last use of BuildFile: the constructor for BuildFileAddress.

[ci skip-rust-tests]
[ci skip-jvm-tests]

# Delete this line to force CI to run Clippy and the Rust tests.
[ci skip-rust-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run Clippy and the Rust tests.
[ci skip-rust-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run Clippy and the Rust tests.
[ci skip-rust-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run Clippy and the Rust tests.
[ci skip-rust-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run Clippy and the Rust tests.
[ci skip-rust-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
Comment on lines -303 to +263
def __init__(
self,
build_file: Optional[BuildFile] = None,
target_name: Optional[str] = None,
rel_path: Optional[str] = None,
) -> None:
def __init__(self, *, rel_path: str, target_name: Optional[str] = None) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a little not-Kosher to make it kwargs-only. But I think it's safe enough to do.

Because rel_path can no longer be optional, it no longer has a default arg. So, it must come before target_name in the params list. This would be dangerous to reorder if people were using positional args, because they're both str. Instead, kwargs-only will make the error more explicit.

I also don't think this will hit many people because build_file was the first positional arg, and you can't give both build_file and rel_path, so I suspect that most people were already using kwargs. Otherwise they would have used BuildFileAddress(None, "target", "src/python/pants").

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

I think this is OK, but let's hear from @stuhood ?

Comment on lines -303 to +263
def __init__(
self,
build_file: Optional[BuildFile] = None,
target_name: Optional[str] = None,
rel_path: Optional[str] = None,
) -> None:
def __init__(self, *, rel_path: str, target_name: Optional[str] = None) -> None:
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

I think this is OK, but let's hear from @stuhood ?

Copy link
Sponsor 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!

@Eric-Arellano Eric-Arellano merged commit 6d745f0 into pantsbuild:master Jun 20, 2020
@Eric-Arellano Eric-Arellano deleted the remove-build-file branch June 20, 2020 23:13
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.

None yet

3 participants