Skip to content

perf: allow to create Link with given filename#941

Open
radoering wants to merge 1 commit into
python-poetry:mainfrom
radoering:perf/link-filename
Open

perf: allow to create Link with given filename#941
radoering wants to merge 1 commit into
python-poetry:mainfrom
radoering:perf/link-filename

Conversation

@radoering
Copy link
Copy Markdown
Member

Extracting the filename from the URL is quite expensive and PEP 691 mandates a filename field anyway.

This can be used downstream to speed up locking by about 15 % in some cases.

  • Added tests for changed code.
  • Updated documentation for changed code.

Extracting the filename from the URL is quite expensive and PEP 691 mandates a `filename` field anyway.
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • In Link.__init__, consider checking if filename is not None instead of if filename: so that falsy but valid filenames (e.g. empty strings) are not silently ignored.
  • Overwriting the filename cached_property by assigning self.filename = filename in __init__ is a bit subtle; it may be clearer to store the provided filename in a private attribute (e.g. self._filename) and have the filename property/cached_property read from that, to avoid instance-level shadowing of the descriptor.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `Link.__init__`, consider checking `if filename is not None` instead of `if filename:` so that falsy but valid filenames (e.g. empty strings) are not silently ignored.
- Overwriting the `filename` cached_property by assigning `self.filename = filename` in `__init__` is a bit subtle; it may be clearer to store the provided filename in a private attribute (e.g. `self._filename`) and have the `filename` property/cached_property read from that, to avoid instance-level shadowing of the descriptor.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

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.

1 participant