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

No way to do operations on marker #448

Open
gaborbernat opened this issue Jul 16, 2021 · 16 comments
Open

No way to do operations on marker #448

gaborbernat opened this issue Jul 16, 2021 · 16 comments

Comments

@gaborbernat
Copy link

gaborbernat commented Jul 16, 2021

Currently, the marker has its data field private, so there's no way for someone to search for markers once parsed a requirement. My goal here is to filter for some extra, roughly:

extras=[r.value for l, _, r in (i for i in a.marker._markers if isinstance(i, tuple) and len(i) == 3) if l.value =='extra']

I propose to either:

  • make ._markers public
  • implement __iter__ that yields entries from _markers

I've needed this also in tox rewrite - https://github.com/tox-dev/tox/blob/rewrite/src/tox/tox_env/python/virtual_env/package/api.py#L186-L211

@uranusjr
Copy link
Member

Could you describe what kind of operations you intend to perform? i.e. the operands and what result you want to get. I’m mostly just curious because my previous attempts on modifying the markers all ended in frustrations and I ended up giving up and just doing things like f"({marker1}) and ({marker2})".

@gaborbernat
Copy link
Author

You can see in the link above I've provided; mostly just extracting and removing the extra marker 😊 By the way for me the extras property is empty, and they show up in the markers instead 🤔

@pradyunsg
Copy link
Member

pradyunsg commented Jul 21, 2021

By the way for me the extras property is empty, and they show up in the markers instead 🤔

This is likely an understanding gap. The extras property maps to the content between the square brackets and is completely independent of how markers are processed.

name[foo]>=2,<3; extra == "bar"

req.extras is a set containing foo. The marker is a different part of the requirement, and is stored in req.marker. The "extra" property in the marker is parsed and stored as part of "things to evaluate when looking at this marker", and is unrelated to "extras of this package to install".

@uranusjr
Copy link
Member

mostly just extracting and removing the extra marker

That makes sense, thanks. +1 to the __iter__ method from me, that sounds like a reasonable and useful addition to inspect a marker. We’ll need to find ways to deal with nested expressions but that should be technically not difficult.

The "extra" property in the marker is parsed and stored as part of "things to evaluate when looking at this marker", and is unrelated to "extras of this package to install".

I just want to add this has always been a great source of confusion to a lot of people and is one of the main reasons I hate extras.

@pradyunsg
Copy link
Member

Yea, I think we can all agree that extras are a very messy thing. :)

@gaborbernat
Copy link
Author

For being able to remove elements would be helpful to just make _markers public 🤔 rather than go down the __iter__ path

@pradyunsg
Copy link
Member

pradyunsg commented Jul 21, 2021

+1 to the __iter__ method from me, that sounds like a reasonable and useful addition to inspect a marker. We’ll need to find ways to deal with nested expressions but that should be technically not difficult.

Hmm... I'm not sure what __iter__ would return. The only thing that I can think of is tokens of the expressions -- (1) that information is not stored currently and (2) I'm not sure that's super useful anyway?

For the kinds of things that @gaborbernat is describing, an expression AST seems more appropriate. That's basically how we store things currently too, handling nesting fairly organically. For exposing that as a public API, I think we basically need to figure out what all we need to expose (I wanna start small and strict), what modifying an AST looks like and how "gimme a marker from this AST" would work.

@gaborbernat
Copy link
Author

Well my MVP functionality would be:

  • Find all markers of type and value X
  • Remove marker of type and value X (note removing a marker is not just remove the marker itself but also any operator it might be attached to).

@uranusjr
Copy link
Member

uranusjr commented Jul 21, 2021

Hmm... I'm not sure what __iter__ would return. The only thing that I can think of is tokens of the expressions -- (1) that information is not stored currently and (2) I'm not sure that's super useful anyway?

I’m thinking something like

>>> marker = Marker("(os_name == 'nt' or sys_platform == 'linux') and python_version <= '3.8'")
>>> submarkers = list(iter(marker))
>>> submarkers
[Marker("os_name == 'nt' or sys_platform == 'linux'"), "and", Marker("python_version <= '3.8'")]
>>> list(iter(submarkers[0]))
[Marker("os_name == 'nt'), "or", Marker("sys_platform == 'linux'")]
>>> list(iter(submarkers[1]))
[Marker("python_version <= '3.8'")]

So yeah, basically an AST, and maybe we can implement a more sophisticated API than just __iter__.

I don’t like exposing Marker._markers directly since pyparsing constructs should be an implementation detail. We should wrap everything exposed publicly in Marker (and maybe add a new type MarkerOperator or something).

@gaborbernat
Copy link
Author

@uranusjr I'm ok with that 👍 As long as we implement del too, so users can remove markers 😊

@uranusjr
Copy link
Member

uranusjr commented Jul 21, 2021

Personally I prefer having Marker immutable instead and provide something like

>>> Marker.join([Marker("os_name == 'nt'"), "or", Marker("sys_platform == 'linux'")])
Marker("os_name == 'nt' or sys_platform == 'linux'")

or perhaps

>>> Marker("os_name == 'nt'") | Marker("sys_platform == 'linux'")
Marker("os_name == 'nt' or sys_platform == 'linux'")

@FFY00
Copy link
Member

FFY00 commented Sep 8, 2021

That would be useful. It would allow us replace usages such as this

requirement = packaging.requirements.Requirement(requirement_string)
if requirement.marker:  # append our extra to the marker
    requirement.marker = packaging.markers.Marker(
        str(requirement.marker) + f' and extra == "{extra}"'
    )
else:  # add our extra marker
    requirement.marker = packaging.markers.Marker(f'extra == "{extra}"')

It could turn into

requirement = packaging.requirements.Requirement(requirement_string)
extra_marker = packaging.markers.Marker(f'extra == "{extra}"')
if requirement.marker:
	requirement.marker += extra_marker
else:
	requirement.marker = extra_marker

Perhaps we could allow empty markers, and do

requirement = packaging.requirements.Requirement(requirement_string)
requirement.marker += packaging.markers.Marker(f'extra == "{extra}"')

(this is code I use when mapping PEP 621 optional-dependencies entries to core metadata)

@brettcannon
Copy link
Member

brettcannon commented Sep 8, 2021

Actually, we could take a cue from sets and provide overrides for & and | (which then includes &= and |=).

@astrojuanlu
Copy link
Contributor

astrojuanlu commented May 22, 2023

Arrived here from https://discuss.python.org/t/programmatically-getting-non-optional-requirements-of-current-directory/26963/5?u=astrojuanlu. If I understand correctly, this would enable users to filter out requirements that have an extras marker, am I right? If not, should I open a different issue?

@layday was advocating suggesting "addding a partial_match flag to evaluate", I'm not 100 % sure if it would cover the same use case.

@layday
Copy link
Member

layday commented May 22, 2023

advocating

Merely suggested ;P

@brettcannon
Copy link
Member

If I understand correctly, this would enable users to filter out requirements that have an extras marker, am I right?

Yes; the idea is the markers on a requirements object would be directly exposed so you can introspect them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants