Turn attributes on HTMLPage into local variables and standalone functions #5826
Conversation
This property is only used in HTMLPage.links, which is only called once per instance in PackageFinder.find_all_candidates(). This would not affect performance or behavior, but improves data locality. The unit test of this property is and modified to test the underlying function instead.
This attribute is only used by HTMLPage.links, which is only used once per instance in PackageFinder.find_all_candidates(), so this change does not affect performance or behavior, but improves data locality.
src/pip/_internal/index.py
Outdated
| @@ -706,24 +706,40 @@ def egg_info_matches( | |||
| return None | |||
|
|
|||
|
|
|||
| def _parse_base_url(document, page_url): | |||
There was a problem hiding this comment.
This is a nit, but I mention it because it will probably come up a bunch more times. For functions that accept parsed html, it might be better not to call the function "parse" because it has already been parsed by the time it receives it. So maybe something like _get_base_url() or _determine_base_url(). (I know I said parse_anchor() as an off-hand example in an earlier comment, so I'm arguing against that.)
There was a problem hiding this comment.
_determine_base_url() sounds good to me :)
src/pip/_internal/index.py
Outdated
| return bases[0].get("href") | ||
| else: | ||
| return self.url | ||
|
|
||
| @property | ||
| def links(self): |
There was a problem hiding this comment.
I see the responsibility of the property is being expanded also to parse. Since normally, properties should "do" very little, you might want to add a comment saying the property should only be called once. (I'm assuming this is just a temporary intermediate step, so I'm okay with leaving it with an expanded scope.)
There was a problem hiding this comment.
I totally agree. This property is only left as-is to reduce unnecessary diff, since the property is going to be removed in the next patch.
There was a problem hiding this comment.
On second thought, I’m going to rename the property (and turn it into a generator function) to clarify its purpose. I hate properties anyway, personally.
uranusjr
force-pushed
the
htmlpage-extract-localize
branch
from
868c21c
to
47ba191
Compare
|
Thanks! :) |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
lock
bot
added
the
auto-locked
#5800 part 1a (part a of #5822).
I also rewrote
parse_base_urlas mentioned in #5822 (comment).