Enable Ruff preview #1045
Enable Ruff preview #1045
Conversation
|
| @@ -15,7 +15,7 @@ def get_files(**kwargs): | |||
| if str(f.path) == 'LICENSE.txt': | |||
| files.append(File(Path(metadata_directory, 'licenses', f.path), f.contents)) | |||
|
|
|||
| if f.path.parts[0] not in (kwargs['package_name'], 'tests'): | |||
| if f.path.parts[0] not in {kwargs['package_name'], 'tests'}: | |||
There was a problem hiding this comment.
It's a cool lint but I think it actively harms performance. In the vast vast vast majority of cases, membership checks are much faster against a tuple literal vs a set literal.
-
A tuple literal is often optimizable to a global singleton and so has 0 construction cost. A set literal is quite expensive to construct every time.
-
Regardless of it being a literal, linearly checking against <5 members is faster than hashing the item and then checking against it in a hashmap.
There was a problem hiding this comment.
I'm glad someone else thinks the way I do! Do you think I should revert this particular one? I would like to.
There was a problem hiding this comment.
Yeah I think just don't apply this lint and move on. There are many questionable lints 😃
There was a problem hiding this comment.
Will do! Which others are you not fond of?
There was a problem hiding this comment.
You're doing great by maintaining your own allowlist of lints 😄 so I have nothing to add
https://github.com/pypa/hatch/blob/977f88285e9747a0962f7a068b92b356043ff88d/ruff.toml
Here's my allowlist
https://github.com/oprypin/mkdocs-literate-nav/blob/95194eb34bd0eb6fcd4432329be7decd4163305b/pyproject.toml#L106
I don't remember which rules I didn't like. When I didn't like them, I just didn't add them, that's it.
There was a problem hiding this comment.
@oprypin it doesn’t harm performance thanks to Python’s optimizer, see astral-sh/ruff#8758
A tuple literal is often optimizable to a global singleton and so has 0 construction cost. A set literal is quite expensive to construct every time.
Not when it’s a set lookup like Ruff creates here (x in {...}), then it gets optimized to a global frozenset just like tuples.
There was a problem hiding this comment.
@flying-sheep but this is a case where the optimizer doesn't kick in
There was a problem hiding this comment.
I see mostly expressions like
if token not in {'or', 'and', 'with', '(', ')'}
version in {'a', 'b', 'c', 'rc', 'alpha', 'beta', 'pre', 'preview'}
which are cases where the optimizer kicks in. Which means that your original comment is wrong:
In the vast vast vast majority of cases, membership checks are much faster against a tuple literal vs a set literal.
The above isn’t true for this code base. You’re right in your latest comment that for this single line (a set literal containing non-literal values), the optimizer doesn’t kick in. But in the vast majority of cases changed in this PR, it does.
There was a problem hiding this comment.
Okay I acquiesce, I won't be ignoring this rule 🙂
No description provided.