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

Upgrade Ruff to 0.6.2 #1588

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Jun 29, 2024

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

  • warning: RUF025 has been remapped to C420.
  • Selection of deprecated rule UP027 is not allowed when preview is enabled.

@@ -174,7 +174,7 @@ def project_metadata_from_core_metadata(core_metadata: str) -> dict[str, Any]:
left, _, right = marker
if left.value == 'extra':
extra = right.value
del markers[i]
del markers[i] # noqa: B909

Choose a reason for hiding this comment

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

This is safe currently because there is always a break after the deletion, but it would be preferable to do this safely rather than suppress the warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could simply extract i in the loop, then use ioutside the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I change that in a different PR? Somehow I cannot get the fix right for now.

Choose a reason for hiding this comment

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

Let me take a quick crack at it...

Choose a reason for hiding this comment

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

I will PR your branch soon later today with a rewrite, I have something working but I have some meetings.

Copy link

@DylanLukes DylanLukes left a comment

Choose a reason for hiding this comment

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

There are numerous rules enabled here that are in preview only. These should probably be disabled?
warning: Selection `E112` has no effect because preview is not enabled.
warning: Selection `E113` has no effect because preview is not enabled.
warning: Selection `E115` has no effect because preview is not enabled.
warning: Selection `E116` has no effect because preview is not enabled.
warning: Selection `E201` has no effect because preview is not enabled.
warning: Selection `E202` has no effect because preview is not enabled.
warning: Selection `E203` has no effect because preview is not enabled.
warning: Selection `E211` has no effect because preview is not enabled.
warning: Selection `E221` has no effect because preview is not enabled.
warning: Selection `E222` has no effect because preview is not enabled.
warning: Selection `E223` has no effect because preview is not enabled.
warning: Selection `E224` has no effect because preview is not enabled.
warning: Selection `E225` has no effect because preview is not enabled.
warning: Selection `E226` has no effect because preview is not enabled.
warning: Selection `E227` has no effect because preview is not enabled.
warning: Selection `E228` has no effect because preview is not enabled.
warning: Selection `E231` has no effect because preview is not enabled.
warning: Selection `E241` has no effect because preview is not enabled.
warning: Selection `E242` has no effect because preview is not enabled.
warning: Selection `E251` has no effect because preview is not enabled.
warning: Selection `E252` has no effect because preview is not enabled.
warning: Selection `E261` has no effect because preview is not enabled.
warning: Selection `E262` has no effect because preview is not enabled.
warning: Selection `E265` has no effect because preview is not enabled.
warning: Selection `E266` has no effect because preview is not enabled.
warning: Selection `E271` has no effect because preview is not enabled.
warning: Selection `E272` has no effect because preview is not enabled.
warning: Selection `E273` has no effect because preview is not enabled.
warning: Selection `E274` has no effect because preview is not enabled.
warning: Selection `E275` has no effect because preview is not enabled.
warning: Selection `E502` has no effect because preview is not enabled.
warning: Selection `W391` has no effect because preview is not enabled.
warning: Selection `B909` has no effect because preview is not enabled.
warning: Selection `FURB110` has no effect because preview is not enabled.
warning: Selection `FURB113` has no effect because preview is not enabled.
warning: Selection `FURB116` has no effect because preview is not enabled.
warning: Selection `FURB118` has no effect because preview is not enabled.
warning: Selection `FURB131` has no effect because preview is not enabled.
warning: Selection `FURB132` has no effect because preview is not enabled.
warning: Selection `FURB142` has no effect because preview is not enabled.
warning: Selection `FURB145` has no effect because preview is not enabled.
warning: Selection `FURB148` has no effect because preview is not enabled.
warning: Selection `FURB152` has no effect because preview is not enabled.
warning: Selection `FURB157` has no effect because preview is not enabled.
warning: Selection `FURB164` has no effect because preview is not enabled.
warning: Selection `FURB166` has no effect because preview is not enabled.
warning: Selection `FURB171` has no effect because preview is not enabled.
warning: Selection `FURB180` has no effect because preview is not enabled.
warning: Selection `FURB192` has no effect because preview is not enabled.
warning: Selection `PLC0415` has no effect because preview is not enabled.
warning: Selection `PLC1901` has no effect because preview is not enabled.
warning: Selection `PLC2701` has no effect because preview is not enabled.
warning: Selection `PLC2801` has no effect because preview is not enabled.
warning: Selection `PLE0304` has no effect because preview is not enabled.
warning: Selection `PLE1141` has no effect because preview is not enabled.
warning: Selection `PLE4703` has no effect because preview is not enabled.
warning: Selection `PLR0202` has no effect because preview is not enabled.
warning: Selection `PLR0203` has no effect because preview is not enabled.
warning: Selection `PLR1733` has no effect because preview is not enabled.
warning: Selection `PLR6104` has no effect because preview is not enabled.
warning: Selection `PLR6201` has no effect because preview is not enabled.
warning: Selection `PLR6301` has no effect because preview is not enabled.
warning: Selection `PLW0108` has no effect because preview is not enabled.
warning: Selection `PLW0177` has no effect because preview is not enabled.
warning: Selection `PLW1514` has no effect because preview is not enabled.
warning: Selection `PLW1641` has no effect because preview is not enabled.
warning: Selection `PLW3201` has no effect because preview is not enabled.
warning: Selection `PYI059` has no effect because preview is not enabled.
warning: Selection `RUF021` has no effect because preview is not enabled.
warning: Selection `RUF022` has no effect because preview is not enabled.
warning: Selection `RUF023` has no effect because preview is not enabled.
warning: Selection `RUF027` has no effect because preview is not enabled.
warning: Selection `RUF028` has no effect because preview is not enabled.
warning: Selection `RUF029` has no effect because preview is not enabled.
warning: Selection `S401` has no effect because preview is not enabled.
warning: Selection `S402` has no effect because preview is not enabled.
warning: Selection `S403` has no effect because preview is not enabled.
warning: Selection `S405` has no effect because preview is not enabled.
warning: Selection `S406` has no effect because preview is not enabled.
warning: Selection `S407` has no effect because preview is not enabled.
warning: Selection `S408` has no effect because preview is not enabled.
warning: Selection `S409` has no effect because preview is not enabled.
warning: Selection `S411` has no effect because preview is not enabled.
warning: Selection `S412` has no effect because preview is not enabled.
warning: Selection `S413` has no effect because preview is not enabled.
warning: Selection `S415` has no effect because preview is not enabled.
warning: Selection `UP042` has no effect because preview is not enabled.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Aug 27, 2024

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.

@DimitriPapadopoulos DimitriPapadopoulos changed the title Upgrade Ruff to 0.6.1 Upgrade Ruff to 0.6.2 Aug 27, 2024
@DylanLukes
Copy link

DylanLukes commented Aug 27, 2024

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
Copy link

DylanLukes commented Aug 27, 2024

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

if (requirements := message.get_all('Requires-Dist')) is not None:
from packaging.requirements import Requirement
dependencies = []
for requirement in requirements:
req = Requirement(requirement)
if req.marker is None:
dependencies.append(str(req))
continue
markers = req.marker._markers # noqa: SLF001
for i, marker in enumerate(markers):
if isinstance(marker, tuple):
left, _, right = marker
if left.value == 'extra':
extra = right.value
del markers[i] # noqa: B909
# If there was only one marker then there will be an unnecessary
# trailing semicolon in the string representation
if not markers:
req.marker = None
# Otherwise we need to remove the preceding `and` operation
else:
del markers[i - 1]
optional_dependencies.setdefault(extra, []).append(str(req))
break
else:
dependencies.append(str(req))
metadata['dependencies'] = dependencies
if optional_dependencies:
metadata['optional-dependencies'] = optional_dependencies
return metadata

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:

def test_dependencies(self):
core_metadata = f"""\
Metadata-Version: {LATEST_METADATA_VERSION}
Name: My.App
Version: 0.1.0
Requires-Dist: bar==5
Requires-Dist: foo==1
Provides-Extra: feature1
Requires-Dist: bar==5; (python_version < '3') and extra == 'feature1'
Requires-Dist: foo==1; extra == 'feature1'
Provides-Extra: feature2
Requires-Dist: bar==5; extra == 'feature2'
Requires-Dist: foo==1; (python_version < '3') and extra == 'feature2'
Provides-Extra: feature3
Requires-Dist: baz@ file:///path/to/project ; extra == 'feature3'
"""
assert project_metadata_from_core_metadata(core_metadata) == {
'name': 'My.App',
'version': '0.1.0',
'dependencies': ['bar==5', 'foo==1'],
'optional-dependencies': {
'feature1': ['bar==5; python_version < "3"', 'foo==1'],
'feature2': ['bar==5', 'foo==1; python_version < "3"'],
'feature3': ['baz@ file:///path/to/project'],
},
}

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
Copy link

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
Copy link
Contributor Author

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

@DylanLukes
Copy link

DylanLukes commented Oct 2, 2024

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
Copy link
Collaborator

ofek commented Oct 2, 2024

I'll look into this soon, sorry

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

Successfully merging this pull request may close these issues.

3 participants