pypa / hatch

Modern, extensible Python project management
https://hatch.pypa.io/latest/
MIT License
6.11k stars 309 forks source link

Upgrade Ruff to 0.6.8 #1588

Closed DimitriPapadopoulos closed 3 weeks ago

DimitriPapadopoulos commented 5 months ago

https://github.com/astral-sh/ruff/tags

DimitriPapadopoulos commented 3 months ago

I ran scripts/update_ruff.py which added them automatically. Doesn't it do the right thing?

Besides, most of them have not been added in this PR. I believe the intent has always been to run ruff check --preview.

DylanLukes commented 3 months ago

I ran scripts/update_ruff.py which added them automatically. Doesn't it do the right thing?

Besides, most of them have not been added in this PR. I believe the intent has always been to run ruff check --preview.

Ah okay! Makes sense they came from the script. The issue I was seeing is that they appear when I run hatch fmt --check which did not occur before?

I believe the intent has always been to run ruff check --preview.

If this is the case maybe something else is going on?

DylanLukes commented 3 months ago

There's actually a more subtle issue with this code as well. The _markers are a tree, not a list.

https://github.com/pypa/hatch/blob/8f1a1d6f26bd48549e9b0eacc76c3d7d03b40423/backend/src/hatchling/metadata/spec.py#L161-L196

It cannot handle cases such as the one described in Marker's own documentation where there are more complicated nestings.

class Marker:
    def __init__(self, marker: str) -> None:
        # Note: We create a Marker object without calling this constructor in
        #       packaging.requirements.Requirement. If any additional logic is
        #       added here, make sure to mirror/adapt Requirement.
        try:
            self._markers = _normalize_extra_values(_parse_marker(marker))
            # The attribute `_markers` can be described in terms of a recursive type:
            # MarkerList = List[Union[Tuple[Node, ...], str, MarkerList]]
            #
            # For example, the following expression:
            # python_version > "3.6" or (python_version == "3.6" and os_name == "unix")
            #
            # is parsed into:
            # [
            #     (<Variable('python_version')>, <Op('>')>, <Value('3.6')>),
            #     'and',
            #     [
            #         (<Variable('python_version')>, <Op('==')>, <Value('3.6')>),
            #         'or',
            #         (<Variable('os_name')>, <Op('==')>, <Value('unix')>)
            #     ]
            # ]
        except ParserSyntaxError as e:
            raise InvalidMarker(str(e)) from e

        ...

We have this example in the tests:

https://github.com/pypa/hatch/blob/8f1a1d6f26bd48549e9b0eacc76c3d7d03b40423/tests/backend/metadata/test_spec.py#L205-L230

But the current code cannot actually handle something like:

Requires-Dist: bar==5; ((python_version < '3') and extra == 'feature1') or ((python_version >= '3') and extra == 'feature2')

This is probably uncommon enough that no one has really run into it? Still, it seems like in general it should be handled. Some sort of visitor is probably the appropriate approach.

Or it might make the most sense to just handle the easy case and throw an error stating that the Requirement is too complex to handle otherwise.

DylanLukes commented 3 months ago

Actually handling all of the possible cases for an arbitrary PEP 508 requirement marker is messy and a rabbit hole. Might be best to just leave it as is until it becomes an issue down the line. It seemingly hasn't yet.

DimitriPapadopoulos commented 2 months ago

I think the new CI failures are unrelated to this PR.

DylanLukes commented 1 month ago

Whatever is causing those CI failures, I hope they can get resolved soon. For the time being I'm just not using hatch fmt...

ofek commented 1 month ago

I'll look into this soon, sorry

DylanLukes commented 3 weeks ago

After this is merged in, I have a PR just about ready to go for Ruff 0.7.x. I'm just working through the details of the changelog and migration guide to make sure there's nothing huge...