-
Notifications
You must be signed in to change notification settings - Fork 2.4k
more typechecking fixes #4755
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
more typechecking fixes #4755
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,13 +5,15 @@ | |
| import warnings | ||
|
|
||
| from collections import defaultdict | ||
| from html import unescape | ||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING | ||
| from typing import Any | ||
| from typing import Dict | ||
| from typing import Iterator | ||
| from typing import List | ||
| from typing import Optional | ||
| from urllib.parse import quote | ||
|
|
||
| import requests | ||
| import requests.auth | ||
|
|
@@ -43,23 +45,6 @@ | |
| if TYPE_CHECKING: | ||
| from poetry.core.packages.dependency import Dependency | ||
|
|
||
| try: | ||
| from html import unescape | ||
| except ImportError: | ||
| try: | ||
| from html.parser import HTMLParser | ||
| except ImportError: | ||
| from HTMLParser import HTMLParser | ||
|
|
||
| unescape = HTMLParser().unescape | ||
|
|
||
|
|
||
| try: | ||
| from urllib.parse import quote | ||
| except ImportError: | ||
| from urllib import quote | ||
|
|
||
|
|
||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore") | ||
| import html5lib | ||
|
|
@@ -164,6 +149,8 @@ def clean_link(self, url: str) -> str: | |
| return self._clean_re.sub(lambda match: "%%%2x" % ord(match.group(0)), url) | ||
|
|
||
|
|
||
| # TODO: revisit whether the LegacyRepository should inherit from PyPiRepository. | ||
| # <https://github.com/python-poetry/poetry/pull/4755#discussion_r748865374>. | ||
| class LegacyRepository(PyPiRepository): | ||
| def __init__( | ||
| self, | ||
|
|
@@ -269,7 +256,7 @@ def find_packages(self, dependency: "Dependency") -> List[Package]: | |
| if self._cache.store("matches").has(key): | ||
| versions = self._cache.store("matches").get(key) | ||
| else: | ||
| page = self._get("/{}/".format(dependency.name.replace(".", "-"))) | ||
| page = self._get_page("/{}/".format(dependency.name.replace(".", "-"))) | ||
| if page is None: | ||
| return [] | ||
|
|
||
|
|
@@ -338,14 +325,14 @@ def package( | |
| return package | ||
|
|
||
| def find_links_for_package(self, package: Package) -> List[Link]: | ||
| page = self._get("/{}/".format(package.name.replace(".", "-"))) | ||
| page = self._get_page("/{}/".format(package.name.replace(".", "-"))) | ||
| if page is None: | ||
| return [] | ||
|
|
||
| return list(page.links_for_version(package.version)) | ||
|
|
||
| def _get_release_info(self, name: str, version: str) -> dict: | ||
| page = self._get("/{}/".format(canonicalize_name(name).replace(".", "-"))) | ||
| page = self._get_page("/{}/".format(canonicalize_name(name).replace(".", "-"))) | ||
| if page is None: | ||
| raise PackageNotFound(f'No package named "{name}"') | ||
|
|
||
|
|
@@ -419,7 +406,7 @@ def _get_release_info(self, name: str, version: str) -> dict: | |
|
|
||
| return data.asdict() | ||
|
|
||
| def _get(self, endpoint: str) -> Optional[Page]: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What was wrong with _get?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the correct solution here is going to be refactoring LegacyRepository to not inherit from PypiRepository. I'd suggest doing that in a follow-up PR, and leaving some
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if that's the right long-term fix, So I'd prefer to leave this change in place: so that we can leave this file mypy-clean - rather than having to whitelist it and allow other errors to slip back in. (I do not expect to follow up with the change that you propose. Not that I disagree with it; but it's more work than I intend to be taking on here.)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's sounds logic -- let's go ahead and add a |
||
| def _get_page(self, endpoint: str) -> Optional[Page]: | ||
| url = self._url + endpoint | ||
| try: | ||
| response = self.session.get(url) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,11 +22,11 @@ def __init__( | |
| if repositories is None: | ||
| repositories = [] | ||
|
|
||
| self._lookup: Dict[str, int] = {} | ||
| self._lookup: Dict[Optional[str], int] = {} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why there is an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure this is unintentional behavior (even if it doesn't cause a bug). For now I'd just whitelist it with an annotation, and circle back to address this on a follow-up PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm in two minds about this.
Well, why not both? Suggest that we leave the annotation in place for now - it does, after all, reflect what the code is actually doing - but also add a (Again, I do not expect to follow-up with any fix for that - this looks like quite a pervasive change).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the compromise here is good. Fair enough! |
||
| self._repositories: List[Repository] = [] | ||
| self._default = False | ||
| self._has_primary_repositories = False | ||
| self._secondary_start_idx = None | ||
| self._secondary_start_idx: Optional[int] = None | ||
|
|
||
| for repository in repositories: | ||
| self.add_repository(repository) | ||
|
|
@@ -65,6 +65,8 @@ def add_repository( | |
| """ | ||
| Adds a repository to the pool. | ||
| """ | ||
| # FIXME: surely it's a problem that the repository name can be None here? | ||
| # All nameless repositories will collide in self._lookup. | ||
| repository_name = ( | ||
| repository.name.lower() if repository.name is not None else None | ||
| ) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.