Skip to content

Conversation

Eric-Arellano
Copy link
Contributor

Improves upon #16420. @huonw wisely suggested we avoid using hash for constraints files because it makes Git diffs and merge conflicts much worse. python-poetry/poetry#496 rust-lang/cargo#7070

[ci skip-rust]
[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 added the category:internal CI, fixes for not-yet-released features, etc. label Aug 10, 2022
@Eric-Arellano Eric-Arellano requested a review from chrisjrn August 10, 2022 18:09
# 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 marked this pull request as ready for review August 10, 2022 19:15
@@ -155,35 +154,41 @@ def warn_python_repos(option: str) -> None:
return MaybeWarnPythonRepos()


@dataclass
class _PipArgsAndConstraintsSetup:
args: list[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

This works since _PipArgsAndConstraintsSetup is private and only a return type of a helper, but having to look at any un-frozen struct, at least for me, raises suspicion reading rule code. If there was some mysterious bug reported and I was scanning the codebase, this would cause me to burn un-needed time reviewing why this is OK. I may be a poor example here though of a code reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I myself was a bit confused trying to trace whether this was safe or not. Thanks

# 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) August 10, 2022 19:49
@Eric-Arellano Eric-Arellano merged commit 318fea7 into pantsbuild:main Aug 10, 2022
@Eric-Arellano Eric-Arellano deleted the constraints-file-dont-use-hash branch August 10, 2022 20:30
@huonw
Copy link
Contributor

huonw commented Aug 10, 2022

Woohoo, thanks for the consideration! 😄

@Eric-Arellano
Copy link
Contributor Author

And thank you for the recommendation! It was a great one, and it shed light on 2 other uncertainties I had re: lockfile metadata for this project :D

cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
…sbuild#16469)

Improves upon pantsbuild#16420. @huonw wisely suggested we avoid using `hash` for constraints files because it makes Git diffs and merge conflicts much worse. python-poetry/poetry#496 rust-lang/cargo#7070

[ci skip-rust]
[ci skip-build-wheels]
@huonw huonw mentioned this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants