Conversation
| # that are most likely to point to the source code first | ||
| urls_to_check: list[str] = [] | ||
| url_names_probably_pointing_to_source = ("Source", "Repository", "Homepage") | ||
|
|
||
| urls_to_check: list[str] = [] | ||
| for url_name in url_names_probably_pointing_to_source: | ||
| if url := project_urls.get(url_name): | ||
| urls_to_check.append(url) | ||
| urls_to_check.append(url) # noqa: PERF401 # False-positive with walrus, this code is the fastest form | ||
| urls_to_check.extend( | ||
| url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source | ||
| [url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source] | ||
| ) | ||
|
|
||
| for url in urls_to_check: |
There was a problem hiding this comment.
Test:
import timeit
setup = """
from itertools import chain
project_urls = {
"Changelog": "https://code.qt.io/cgit/pyside/pyside-setup.git/tree/doc/changelogs",
"Documentation": "https://doc.qt.io/qtforpython",
"Homepage": "https://pyside.org",
"Repository": "https://code.qt.io/cgit/pyside/pyside-setup.git/",
"Tracker": "https://bugreports.qt.io/projects/PYSIDE",
}
url_names_probably_pointing_to_source = ("Source", "Repository", "Homepage")
"""
stmt = """
urls_to_check: list[str] = []
for url_name in url_names_probably_pointing_to_source:
if url := project_urls.get(url_name):
urls_to_check.append(url)
urls_to_check.extend(
url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source
)
for url in urls_to_check:
pass
"""
print(timeit.timeit(stmt=stmt, setup=setup))
stmt = """
urls_to_check: list[str] = []
for url_name in url_names_probably_pointing_to_source:
if url := project_urls.get(url_name):
urls_to_check.append(url)
urls_to_check.extend(
[url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source]
)
for url in urls_to_check:
pass
"""
print(timeit.timeit(stmt=stmt, setup=setup))
stmt = """
project_urls_probably_pointing_to_source = (
project_urls.get(url_name) for url_name in url_names_probably_pointing_to_source
)
urls_to_check = chain(
(url for url in project_urls_probably_pointing_to_source if url),
(url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source),
)
for url in urls_to_check:
pass
"""
print(timeit.timeit(stmt=stmt, setup=setup))
stmt = """
project_urls_probably_pointing_to_source = [project_urls.get(url_name) for url_name in url_names_probably_pointing_to_source]
urls_to_check = (
[url for url in project_urls_probably_pointing_to_source if url]
+ [url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source]
)
for url in urls_to_check:
pass
"""
print(timeit.timeit(stmt=stmt, setup=setup))Results (Python 3.10):
0.6867717999994056
0.610139699998399
0.9288775999993959
0.7600129999991623Results (Python 3.13):
0.7435727000010957
0.5391235000024608
0.9949863000001642
0.5781141000006755There was a problem hiding this comment.
Nevermind, I was just missing parenthesis: astral-sh/ruff#15047 (comment)
(new run time on python 3.13 is 0.5153441999973438)
hauntsaninja
left a comment
There was a problem hiding this comment.
This is harder to read. In general, I'm not sure we need this level of linting on typeshed
| urls_to_check = [url for url_name in url_names_probably_pointing_to_source if (url := project_urls.get(url_name))] + [ | ||
| url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source | ||
| ) | ||
| ] |
There was a problem hiding this comment.
I'd prefer formatting as follow but Black doesn't allow:
urls_to_check = (
[url for url_name in url_names_probably_pointing_to_source if (url := project_urls.get(url_name))]
+ [url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source]
)|
I don't feel like anything in typeshed is performance-critical such that we need to worry about micro-optimising specific functions in our tests; I agree that these rules don't really make sense for this repository |
|
From the comments and upvotes here, I think there's sufficient opposition from the core team here that we probably won't be going ahead with this one :-) |
Ref #13295
https://docs.astral.sh/ruff/rules/#perflint-perf
That manual-list-comprehension (PERF401)
initially looked like a false-positiveI just had to add a parenthesis and make everything list comprehensions. I'll let the numbers talk for themselves in comments.About try-except-in-loop (PERF203), here's prior conversation with @AlexWaygood : astral-sh/ruff#12805 (comment)