feat: add API for markers as a structured tree

[danyeaw]

Proposed fix for #448. Based on converting markers between the PyPI and conda ecosystems in https://github.com/conda/conda-pypi/blob/main/conda_pypi/markers.py, this is a proposed API that exposes Marker.root / Marker.as_ast() returning MarkerCompare, MarkerAnd, and MarkerOr nodes so callers can walk markers without using private _markers.

[henryiii]

No need to add a dependency, this import can be TYPE_CHECKING only.

[danyeaw]

Hey @henryiii, thanks so much for the review and feedback!

[henryiii]

I can rebase if you want me to.

[danyeaw]

Hey @henryiii, I noticed when I was rebasing that the tests from #1146 weren't in a test class like the other tests in the module. I refactored them in to a TestMarkerOperators class to improve style consistency with the other tests. However, if that was intentional to add them as test functions I can revert it. 👍

[henryiii]

No, that's fine, thanks!

[henryiii]

I'd like one more review if possible since it's new API, but at least I like it, and @sirosen also liked it (quoted below from Discord).

I only read through #1145 pretty briefly, but it's an API that I would like using. It does seem a little strange that the canonical internal representation is a list of strings, but then a tree can be reconstituted from it. But maybe that's no big deal? I don't know enough about the marker parsing to say if it's cause for concern.
Just as a user giving feedback on "as_ast()", I'd say it looks good.

[sirosen]

As a sample use-case, I have a tool I'm quietly working on for inspecting and rewriting project metadata (defined more broadly than just python package metadata), and one of the gotchas I ran into is the mismatch between project.optional-dependencies and Requires-Dist data for the dynamic case.

I implemented a thing which reads the marker string and tries to grab extra == as a prefix or suffix, but getting a parse tree would be much nicer.

[notatallshaw]

I found a couple of issues.

Firstly a design issue:

>>> print(Marker('python_version >= "3.10"').as_ast())
MarkerCompare(left='python_version', op='>=', right='3.10')

>>> print(Marker('"3.10" <= python_version').as_ast())
MarkerCompare(left='3.10', op='<=', right='python_version')

Both these examples are valid syntax and because the API is only returning strings for operands, and throwing away which is the variable and which is the literal, all consumers of this API will need to reimplement the part of the PEP that defines what are variable names.

I understand the appeal of the simplicity of strings, but I think we need to distinguish the operands in some way here.

Second a type hint issue:

    def as_ast(self) -> MarkerNode | None:
        ...

I don't think as_ast() can ever produce None as Marker("") raises, and you would have to mess with the internal state of markers to ever produce an empty marker list, maybe we can just add an assert is not None before returning the MarkerNode?

[henryiii]

Technically, unless we accept #1126, only one of those is valid. ;) We could also change the standard, that's being discussed. Could we make this always store as variable, op, value, even if it is written the other way?

Since there also needs to be a follow-up (to convert marker trees back into markers), I think it's fine to delay this to 26.2.

[notatallshaw]

Technically, unless we accept #1126, only one of those is valid. ;) We could also change the standard, that's being discussed. Could we make this always store as variable, op, value, even if it is written the other way?

Ah I see, I'm a bit behind on this marker discussion.

Given this is a new API, if consider RHS variables invalid and we keep this string implementation then I'd like to raise a ValueError on calling to_ast() so the result is unambiguous?

[henryiii]

I think if we put this in after #1126, that already has the mechanism to flip sides, which we could use here. And if we change the standard to match the current behavior instead, then we should raise an error early. Let's open a discussion on this soon(ish). (Probably after release, to stay focused).