add Marker.apply(environment) to partially evaluate a marker

[radoering]

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

This is required by poetry-plugin-export, which previously used a combination of validate and without_extras, which only worked in some cases.

Updating poetry-core to 2.4.1 in the poetry repo in python-poetry/poetry#10921 revealed that #943 partially breaks and partially fixes poetry-plugin-export.

It breaks https://github.com/python-poetry/poetry-plugin-export/blob/65172a089497c4dc1e0da020db637b0376ea25c1/tests/test_exporter_pylock_toml.py#L955-L960

It fixes https://github.com/python-poetry/poetry-plugin-export/blob/65172a089497c4dc1e0da020db637b0376ea25c1/tests/test_exporter_pylock_toml.py#L973-L978 and reveals that the expectation is wrong - it should be "*".

I think we do not have a suitable method (or methods to combine) in poetry-core to achieve what is really required in poetry-plugin-export yet.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• Consider documenting explicitly in BaseMarker.apply that environment=None is treated as a no-op (returns an equivalent marker), and how that differs from passing an empty mapping, so callers have a clear contract when choosing between the two.
• In MultiMarker.apply and MarkerUnion.apply, you might want to short-circuit and return self when environment is None to avoid rebuilding an equivalent marker tree unnecessarily.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider documenting explicitly in `BaseMarker.apply` that `environment=None` is treated as a no-op (returns an equivalent marker), and how that differs from passing an empty mapping, so callers have a clear contract when choosing between the two.
- In `MultiMarker.apply` and `MarkerUnion.apply`, you might want to short-circuit and return `self` when `environment is None` to avoid rebuilding an equivalent marker tree unnecessarily.

## Individual Comments

### Comment 1
<location path="tests/version/test_markers.py" line_range="1640-1645" />
<code_context>
+        ("extra == 'a' or extra == 'b'", {}, "extra == 'a' or extra == 'b'"),
+    ],
+)
+def test_apply(
+    marker_string: str, environment: dict[str, str] | None, expected: str
+) -> None:
+    m = parse_marker(marker_string)
+
+    assert m.apply(environment) == parse_marker(expected)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding targeted regression-style cases that mirror the poetry-plugin-export scenarios mentioned in the PR description.

Since this change addresses regressions in `poetry-plugin-export`, it would help to add tests that use those exact marker strings (from `test_exporter_pylock_toml.py`) and assert the expected `apply` output for the relevant environments. That way, the specific real-world failures that motivated this change are covered directly and the intent of the tests is clearer for future maintainers.

Suggested implementation:

```python
        ("extra != 'a' and extra != 'b'", {}, "extra != 'a' and extra != 'b'"),
        ("extra == 'a' or extra == 'b'", {"extra": ("a",)}, ""),
        ("extra == 'a' or extra == 'b'", {"extra": ("c",)}, EMPTY),
        ("extra == 'a' or extra == 'b'", {}, "extra == 'a' or extra == 'b'"),
        # Regression-style cases mirroring poetry-plugin-export scenarios:
        # combined python_version/extra markers that previously misbehaved.
        (
            "python_version >= '3.11' and extra == 'foo'",
            {"python_version": "3.11", "extra": ("foo",)},
            "",
        ),
        (
            "python_version >= '3.11' and extra == 'foo'",
            {"python_version": "3.10", "extra": ("foo",)},
            EMPTY,
        ),
        (
            "(python_version >= '3.11' and extra == 'foo') or extra == 'bar'",
            {"python_version": "3.11", "extra": ("bar",)},
            "",
        ),
        (
            "(python_version >= '3.11' and extra == 'foo') or extra == 'bar'",
            {"python_version": "3.10", "extra": ("baz",)},
            EMPTY,
        ),
    ],
)

```

To make these tests align exactly with the regressions that motivated the PR:

1. Open `tests/export/test_exporter_pylock_toml.py` (or the corresponding exporter test module in your repo) and locate the marker strings used in the failing/export-related cases.
2. Replace the placeholder marker strings in the new test cases:
   * `"python_version >= '3.11' and extra == 'foo'"`
   * `"(python_version >= '3.11' and extra == 'foo') or extra == 'bar'"`

   with the exact marker strings from the exporter tests (including any additional constraints like `sys_platform`, `implementation_name`, etc.).
3. Adjust the expected `apply` outputs (`""` vs `EMPTY` vs a simplified marker string) so they match the actual behavior of `Marker.apply` for those exact markers and environments. You can confirm by running the tests or by manually calling `parse_marker(marker).apply(env)` in a REPL.
4. Optionally, add comments above each new case referencing the specific exporter test name or scenario (e.g., “mirrors test_exporter_pylock_toml::test_export_with_extra_and_python_marker”) to make the regression link explicit for future maintainers.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[radoering]

@dimbleby I would appreciate if you can take a look - especially if I am right that the use case of poetry-plugin-export cannot be covered with existing methods and if this new method makes sense to you.

In https://github.com/python-poetry/poetry-plugin-export/blob/65172a089497c4dc1e0da020db637b0376ea25c1/src/poetry_plugin_export/walker.py#L276-L279 we want a marker without extras but not what without_extras() does but with extra-sub-markers validated. With the new method we can fix it as follows:

-        if not marker.validate({"extra": extras}):
-        marker = marker.without_extras()
-            continue
+        marker = marker.apply({"extra": extras})
+        if marker.is_empty():
+            continue

[dimbleby]

Looks like a reasonable API to have.

exclude() and without_extras() have always been confusing to me - I guess this sequence of pull requests shows that I am not the only one. Perhaps they should (eventually?) be deprecated in favour of this API?

Or anyway they could be rewritten so that they are implemented - on the base marker - via this API.

Do we need the environment = None case? Feels like not.

The other possibility that crossed my mind was that maybe this should not be necessary at all in the export plugin. IIRC, pylock.toml allows markers that include extras. So an alternative spec for the behaviour of the export plugin would be:

• do not accept an argument describing the extras to account for
• instead output a lockfile that encodes "include this package if that extra was set", and allow installers to take care of it

But (i) that would be a breaking change which might or might not be ok and (ii) it has been a while since I looked at the export plugin and I do not know how feasible that would be.

[radoering]

Do we need the environment = None case? Feels like not.

Good point. I just took it from validate() without thinking about it. I think I will remove it.

The other possibility [...]

I thought about this option when implementing the pylock.toml export in poetry-plugin-export and decided for the current version due to the symmetry with the requirements.txt export. Further, I think extra is not allowed in pylock.toml and has to be converted to extras. It is a bit hidden in the spec, but I found a reference at the end of https://packaging.python.org/en/latest/specifications/dependency-specifiers/#defined-environment-marker-fields:

The legacy extra comparison syntax is NOT permitted in lock file packages.marker fields, and installation tools SHOULD reject lock files containing such comparisons as invalid.

Thus, it requires some more effort to be able to export markers with extras.

But (i) that would be a breaking change which might or might not be ok

I would probably add it as option and not as replacement of the current behavior.

[radoering]

exclude() and without_extras() have always been confusing to me - I guess this sequence of pull requests shows that I am not the only one. Perhaps they should (eventually?) be deprecated in favour of this API?

I agree that it is a bit confusing and I am not sure if their usage is always correct. Maybe, it is exactly what is required in some places. Maybe not. That is something we probably should investigate at some point...