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.

Sign up for GitHub

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

Jump to bottom

Fix extracting extras from markers with many extras #2792

Merged
merged 2 commits into from Dec 31, 2022
Merged

Fix extracting extras from markers with many extras #2792

merged 2 commits into from Dec 31, 2022

Conversation

q0w
Copy link
Contributor

@q0w q0w commented Dec 30, 2022

Thanks for contribution

Please, make sure you address all the checklists (for details on how see
development documentation)!

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

@q0w
Copy link
Contributor Author

q0w commented Dec 30, 2022 •
edited

What should be done with requirements like 'sphinx>=3; extra == "extras1" and extra == "extras2"'

requires = [
        Requirement('filelock<4.0.0,>=3.9.0; extra == "extras1" or extra == "extras2"'),
        Requirement('sphinx>=3; extra == "extras1" and extra == "extras2"')
    ]
result = dependencies_with_extras(requires, {"extras1"}, "")

What result is expected?

@q0w
Copy link
Contributor Author

q0w commented Dec 30, 2022 •
edited

Maybe its a bug in the poetry build backend, because setuptools makes different METADATA Requires-Dists

Provides-Extra: extras1
Requires-Dist: filelock (<4.0.0,>=3.9.0) ; extra == 'extras1'
Provides-Extra: extras2
Requires-Dist: filelock (<4.0.0,>=3.9.0) ; extra == 'extras2'

@gaborbernat
Copy link
Member

What result is expected?

I'd say only filelock to show up, as sphinx needs both extra enabled.

Maybe its a bug in the poetry build backend, because setuptools makes different METADATA Requires-Dists

Worth reporting it to them, but I think is valid to expect if something generates such to behave as my previous paragraph here describes. Can you add a test for it?

gaborbernat
gaborbernat requested changes Dec 30, 2022
View reviewed changes
tests/tox_env/python/virtual_env/package/test_python_package_util.py Outdated
@@ -51,6 +51,15 @@ def test_load_dependency_many_extra(pkg_with_extras: PathDistribution) -> None:
assert str(left) == str(right)


def test_load_dependency_poetry_many_extra() -> None:
requires = [Requirement('filelock<4.0.0,>=3.9.0; extra == "extras1" or extra == "extras2"')]
Copy link
Member

Choose a reason for hiding this comment

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

instead of naming it poetry let's just say it extra_or 👍 And lets add an extra_and variant please?

q0w and others added 2 commits December 30, 2022 16:22
@q0w @gaborbernat
Fix extracting extras from markers with many extras
c5bb228
@gaborbernat
PR feedback
5ccf2b3 
Signed-off-by: Bernát Gábor <gaborjbernat@gmail.com>
gaborbernat
gaborbernat approved these changes Dec 31, 2022
View reviewed changes
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Let's ship as is and do the and handling as a follow-up.

@gaborbernat gaborbernat merged commit 196b20d into tox-dev:main Dec 31, 2022
@q0w
Copy link
Contributor Author

q0w commented Jan 1, 2023 •
edited

@gaborbernat How many logic markers should tox handle? One?

@mikelane mikelane mentioned this pull request Jan 2, 2023
Merged
descope bot added a commit to descope/django-descope that referenced this pull request Jan 14, 2023
@descope
chore(deps): update dependency tox to v4.1.2 (#66)
e4ecc86 
This PR contains the following updates:

| Package | Type | Update | Change | Pending |
|---|---|---|---|---|
| [tox](https://togithub.com/tox-dev/tox)
([changelog](https://tox.wiki/en/latest/changelog.html)) | dev | patch |
`4.1.1` -> `4.1.2` | `4.2.8` (+9) |

---

### Release Notes

<details>
<summary>tox-dev/tox</summary>

### [`v4.1.2`](https://togithub.com/tox-dev/tox/releases/tag/4.1.2)

[Compare Source](https://togithub.com/tox-dev/tox/compare/4.1.1...4.1.2)

#### What's Changed

- Fix --skip-missing-interpreters by
[@&#8203;q0w](https://togithub.com/q0w) in
[tox-dev/tox#2793
- Show installed packages after setup in CI envs by
[@&#8203;gaborbernat](https://togithub.com/gaborbernat) in
[tox-dev/tox#2794
- Fix extracting extras from markers with many extras by
[@&#8203;q0w](https://togithub.com/q0w) in
[tox-dev/tox#2792

**Full Changelog**: tox-dev/tox@4.1.1...4.1.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC45OS4yIiwidXBkYXRlZEluVmVyIjoiMzQuOTkuMiJ9-->

Co-authored-by: descope[bot] <descope[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaborbernat gaborbernat gaborbernat approved these changes

Assignees
No one assigned
Labels
bot:chronographer:provided
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@q0w @gaborbernat