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

Rename PythonModule to PythonModuleOwnersRequest #14276

Merged
merged 4 commits into from
Jan 27, 2022

Conversation

Eric-Arellano
Copy link
Contributor

Follow up to #14034. For python_test and pex_binary, we now use infer dependencies on python_requirement targets from the same resolve. (If [python].enable_resolves = true). This implements the feature sketched out at #13621.

Follow ups will apply this restriction to python_source, python_aws_lambda, and python_google_cloud_function. I have no idea how this should work with python_distribution though...

Note that you can only set one resolve, even though python_source for now has the compatible_resolves field to let you have >1. See discussion at #14034 (comment).

# 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
Copy link
Contributor Author

FYI @thejcannon , this touches some code you've been working on. Speaking of which, any thoughts if this should be "internal" vs Plugin API Change for the changelog? Do we think anyone is using PythonModule -> PythonModuleOwners in their plugins?

Comment on lines +198 to +200
resolve_field = tgt[PythonResolveField]
resolve_field.validate(python_setup)
resolve = resolve_field.value_or_default(python_setup)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like how many lines this is taking to do. I'm thinking about ways to simplify this.

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.

Thanks!

@thejcannon
Copy link
Member

FYI @thejcannon , this touches some code you've been working on. Speaking of which, any thoughts if this should be "internal" vs Plugin API Change for the changelog? Do we think anyone is using PythonModule -> PythonModuleOwners in their plugins?

Strictly speaking, feel free to call it out as an API change because it is. I have no issues with the current instability because it results in drastic improvements. However, instability without documentation is worse than instability with docs. 😉

And because of the instability (and the docs needing TLC) I'd imagine most (if not all) plugin authors have messaged in Slack at one time or another. So I'd say if you're not aware of anyone using that mapping then there's likely no one using it.

@Eric-Arellano Eric-Arellano changed the title [internal] Limit Python dependency inference to the target's resolve Rename PythonModule to PythonModuleOwnersRequest Jan 27, 2022
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano merged commit a06684e into pantsbuild:main Jan 27, 2022
@Eric-Arellano Eric-Arellano deleted the py-module-owner branch January 27, 2022 22:46
@wisechengyi wisechengyi mentioned this pull request Jan 29, 2022
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.

3 participants