Fix potentially unbound issues in stubsabot #9754
Conversation
hauntsaninja
left a comment
hauntsaninja
left a comment
There was a problem hiding this comment.
Let's keep the diff_analysis where it was so we don't have to do work for obsolete stubs
Oh yeah, I see. New commit should do that and still avoid "potentially unbound". |
scripts/stubsabot.py
Outdated
| *_, diff_url = diff_info | ||
| links["Diff"] = diff_url |
There was a problem hiding this comment.
I assume keeping the diff_url variable here is good for readability.
Although maybe this could be better if the returned tuple grows in the future?
| *_, diff_url = diff_info | |
| links["Diff"] = diff_url | |
| diff_url = diff_info[3] | |
| links["Diff"] = diff_url |
There was a problem hiding this comment.
I just realized you can do *_, links["Diff"] = diff_info. The worst of both worlds 😃 (looks neat though)
| async def find_first_release_with_py_typed(pypi_info: PypiInfo, *, session: aiohttp.ClientSession) -> PypiReleaseDownload: | ||
| async def find_first_release_with_py_typed(pypi_info: PypiInfo, *, session: aiohttp.ClientSession) -> PypiReleaseDownload | None: | ||
| release_iter = pypi_info.releases_in_descending_order() | ||
| first_release_with_py_typed: PypiReleaseDownload | None = None |
There was a problem hiding this comment.
Let's not change the signature of this method and just have it assert that the return value is not None. This really should only be called with the precondition that release_contains_py_typed was True, otherwise we'll just download all packages for all time
There was a problem hiding this comment.
Alternatively I'd also be fine with moving release_contains_py_typed into here
There was a problem hiding this comment.
Wait, sorry I read a thing wrong, your change was fine as is!
scripts/stubsabot.py
Outdated
|
|
||
| obsolete_since: PypiReleaseDownload | None = None | ||
| is_obsolete = await release_contains_py_typed(latest_release, session=session) | ||
| if is_obsolete: |
There was a problem hiding this comment.
nit: let's get rid of is_obsolete and directly if await contains pytyped
There was a problem hiding this comment.
Alright, wasn't sure if you'd like to keep the variable name just for readability.
scripts/stubsabot.py
Outdated
| diff_info = await get_diff_info(session, stub_info, pypi_info, relevant_version) | ||
| if diff_info is not None: | ||
| github_repo_path, old_tag, new_tag, diff_url = diff_info | ||
| *_, diff_url = diff_info |
There was a problem hiding this comment.
diff_info is already a namedtuple, so let's use attribute access in both places instead of futzing with unpacking logic.
There was a problem hiding this comment.
Didn't see it was a named tuple. Yeah that'll be much better, even than teh original code.
scripts/stubsabot.py
Outdated
|
|
||
| async def find_first_release_with_py_typed(pypi_info: PypiInfo, *, session: aiohttp.ClientSession) -> PypiReleaseDownload: | ||
| async def find_first_release_with_py_typed( | ||
| pypi_info: PypiInfo, latest_release: PypiReleaseDownload, *, session: aiohttp.ClientSession |
There was a problem hiding this comment.
Technically latest_release can be obtained with pypi_info.get_latest_release(). But that would result in calling get_latest_release twice because it's also used for an early check.
I could leave this as-is, change the first parameter to expect the iterator of releases instead, or "cache" get_latest_release (for example by making it private and exposing it through a property that calls _get_latest_release only once), or any other idea.
There was a problem hiding this comment.
Maybe it could return a bit less early, and use release_iter = pypi_info.releases_in_descending_order(). With the releases_in_descending_order moving in find_first_release_with_py_typed, building an iterator early is probably much better than a redundant call to pypi to get the latest release twice
hauntsaninja
left a comment
hauntsaninja
left a comment
There was a problem hiding this comment.
Thanks, this looks good!
2 participants
Fixes "potentially unbound" type issues in stubsabot.
Simplify a bit the section about finding the first
py.typedversion and whether the stub is obsolete.