-
-
Notifications
You must be signed in to change notification settings - Fork 638
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 poetry_requirements
macro for Poetry support
#12174
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
# 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]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting stuff!
src/python/pants/backend/python/macros/poetry_requirement_test.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/macros/poetry_requirement_test.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/macros/poetry_requirement_test.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]
…into poetry_support [ci skip-rust] [ci skip-build-wheels]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shaping up really well! Good work :)
src/python/pants/backend/python/macros/poetry_requirements_test.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/macros/poetry_requirements_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good! One blocking comment.
It's particularly helpful that the tests are thorough, because that would ease porting the parsing to a parsing library at some point in the future.
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
@benjyw Should be complete -- if you have time, your input would be great to have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Other than two small code style things + adding tests for versions like 2.2.0.dev0
, this looks excellent and ready to land.
proj_name: str, attributes: str | dict[str, Any] | list[dict[str, Any]], fp: str | ||
) -> tuple[Requirement, ...]: | ||
if isinstance(attributes, str): | ||
# E.g. `foo = "~1.1~'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# E.g. `foo = "~1.1~'. | |
# E.g. `foo = "~1.1~"`. |
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
("~1.0.0rc0", ">=1.0.0,<1.1.0"), | ||
("^1.0.0rc0", ">=1.0.0,<2.0.0"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that these are wrong, should likely be >=1.0.0rc0
for both. In the production code, that can be fixed by restoring your old min_version
code as the raw string w/ prefix chopped off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caret/tilde operators are somewhat variable; I have this as agreeing with NPM at the moment...I wasn't 100% sure how to check this with Poetry.
An easier fix vs going back to prior code is just to use .public
instead of .base_version
I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks like .public
does the trick!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump on this and fixing the tests. Lgtm otherwise :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh oops -- changed now; thought I was supposed to leave to match NPM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'm not sure why NPM does that, this seems like more correct behavior
Co-authored-by: Eric Arellano <[email protected]>
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…try_support [ci skip-build-wheels]
@@ -1511,6 +1511,8 @@ def compute_pantsd_invalidation_globs( | |||
# macros should be adapted to allow this dependency to be automatically detected. | |||
"requirements.txt", | |||
"3rdparty/**/requirements.txt", | |||
"pyproject.toml", | |||
"3rdparty/**/pyproject.toml", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt anyone will put a pyproject.toml
under 3rdparty
, but it doesn't hurt to have this.
poetry_requirements
macro for Poetry support
Closes #10655.