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 pyopenssl, parametrized, and wheel dependencies #10023

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Jun 10, 2020

pyopenssl was only being used to pin a transitive dep back in 2017. We don't need to do this anymore because requests is more up-to-date. We should also be using a constraints file for this purpose, rather than requirements.txt.

parametrized is similar to Pytest's parametrization feature. Because we want to encourage tests to use Pytest-style, it's better to use that instead. While Pytest cannot parametrize class-based tests, those can be parametrized through other means like inner helper functions.

wheel was not being used by anything.

This was only being used to pin a transitive dep. We don't need to do this anymore because `requests` is more up-to-date.
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
It's better to use Pytest parametrization. Note that this only works with top-level modules, but that's okay because we want to encourage this style for all new tests. Tests that still need to use class-based tests can parametrize through other means like inner helper functions.
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
@@ -3,13 +3,12 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No changes made to this file beyond converting from unitTest-style to Pytest-style.

We don't use it for anything
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
@Eric-Arellano Eric-Arellano changed the title Remove pyopenssl and parametrized dependencies from pantsbuild.pants Remove pyopenssl, parametrized, and wheel dependencies from pantsbuild.pants Jun 10, 2020
@Eric-Arellano Eric-Arellano changed the title Remove pyopenssl, parametrized, and wheel dependencies from pantsbuild.pants Remove pyopenssl, parametrized, and wheel dependencies Jun 10, 2020
@stuhood
Copy link
Member

stuhood commented Jun 10, 2020

Hm, re: parameterized though: if that isn't actually depended on by the core, it won't be a dependency of the core pants wheel. The setup-py task doesn't take the requirements file wholesale.

@jsirois
Copy link
Contributor

jsirois commented Jun 10, 2020

Hm, re: parameterized though: if that isn't actually depended on by the core, it won't be a dependency of the core pants wheel. The setup-py task doesn't take the requirements file wholesale.

The core Pants wheel does not contain Pants own test code anyhow so should never carry dependencies that are only used by Pants own tests.

More generally, if its valid to remove a Pants dependency (tests run, etc.) for Pants own sake, then its valid generally and will be properly reflected in published dists automatically.

I may be missing something about your concern though.

@Eric-Arellano
Copy link
Contributor Author

Ah, good point Stu. I think, John, that he was clarifying a false claim about this resulting in a smaller distribution to end-users.

But, yeah, regardless of that benefit, I think we should still remove parametrized because we want people writing Pytest-style tests instead.

@jsirois
Copy link
Contributor

jsirois commented Jun 10, 2020

I think, John, that he was clarifying a false claim about this resulting in a smaller distribution to end-users.

Aha - the last sentence of your description. Everything makes sense now. Watch the use of too many justifications! False ones are super confusing.

@Eric-Arellano Eric-Arellano merged commit c7923f1 into pantsbuild:master Jun 10, 2020
@Eric-Arellano Eric-Arellano deleted the remove-some-reqs branch June 10, 2020 23:36
@asherf
Copy link
Member

asherf commented Jun 11, 2020

yay! thanks!

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.

4 participants