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

Fix misuse of pretty_constraint, fix downstream test (poetry-core) #4932

Merged

Conversation

radoering
Copy link
Member

Pull Request Check List

Resolves: #4919

The pretty_constraint fix in python-poetry/poetry-core@af08f1c broke a downstream test (test_exporter_can_export_requirements_txt_with_nested_packages_and_markers_any), exposing a misuse of the fact that pretty_constraint was not updated when setting the constraint. This PR fixes the downstream test by fixing the misuse.

  • Added tests for changed code.
  • Updated documentation for changed code.

@finswimmer finswimmer requested a review from a team December 30, 2021 21:07
@@ -69,13 +70,21 @@ def _export_requirements_txt(
content = ""
dependency_lines = set()

for dependency_package in self._poetry.locker.get_project_dependency_packages(
project_requires=self._poetry.package.all_requires, dev=dev, extras=extras
for package, groups in itertools.groupby(
Copy link
Member

Choose a reason for hiding this comment

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

Can you walk me through this set of changes and why they are needed? I can follow the line-by-line, but I'm not sure I'm groking the bigger picture.

Also, this file is going to be removed in favor of the exporter plugin. Can you please make a PR against that repo as well?

Copy link
Member Author

@radoering radoering Jan 19, 2022

Choose a reason for hiding this comment

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

If you are wondering: I accidentally deleted my branch while porting the fix to the exporter plugin. Sorry about that. 😅

The changes in exporter.py are required because get_project_dependency_packages returns objects of type DependencyPackage, which consist of a Dependency and a Package. There can be multiple dependency packages containing the same package for different dependencies, e.g. package a(1.1) for dependency a(>=1.0) with markers sys_platform == "win32" and package a(1.1) for dependency a(>=1.1) with markers sys_platform == "linux". The changes are required in order to receive only one entry for a==1.1 in requirements.txt instead of two entries with different markers.

This behaviour can be observed in the following test cases:

test_exporter_can_export_requirements_txt_with_nested_packages_and_multiple_markers

expected:

bar==7.8.9 ; platform_system != "Windows" or platform_system == "Windows"

without changes:

bar==7.8.9 ; platform_system != "Windows"
bar==7.8.9 ; platform_system == "Windows"

test_exporter_can_export_requirements_txt_with_nested_packages_and_markers_any

expected:

a==1.2.3

without changes:

a==1.2.3
a==1.2.3 ; python_version < "3.8"

test_exporter_can_export_requirements_txt_pyinstaller

expected:

altgraph==0.17

without changes:

altgraph==0.17
altgraph==0.17 ; sys_platform == "darwin"

@radoering radoering closed this Jan 19, 2022
@radoering radoering deleted the fix-pretty-constraint-misuse branch January 19, 2022 05:48
@radoering radoering restored the fix-pretty-constraint-misuse branch January 19, 2022 05:53
@radoering radoering reopened this Jan 19, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
@radoering radoering deleted the fix-pretty-constraint-misuse branch November 24, 2024 12:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

poetry-core 'downstream' tests are failing
2 participants