Fix logic and message reported for version if inaccessible. #701
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR includes some handling of instances where pypi.org is offline or inaccessible. I also cleaned up some of the naming and introduced an exception dataclass to make handling this issue more clear. Also also, I removed the dependence on the six module to instead use the provided urllib urlopen method. Closes #700
Codecov Report
@@ Coverage Diff @@
## master #701 +/- ##
=======================================
Coverage 29.23% 29.24%
=======================================
Files 10 10
Lines 1245 1248 +3
Branches 184 185 +1
=======================================
+ Hits 364 365 +1
- Misses 858 860 +2
Partials 23 23
Continue to review full report at Codecov.
|
ibnesayeed
requested changes
Jul 13, 2020
There was a problem hiding this comment.
I think reading installed version is less likely to every fail as it is already loaded in the memory and is available as an imported attribute, so no need to initialize it as None. Moreover, we do not need to overload the function that checks for the latest version to also compare it with the installed version. Let's do it something like this:
def get_latest_version():
# Check PyPI online, parse the returned JSON, and return just the version number string
# In case of any exception, return None
def check_for_update():
latest = get_latest_version()
if not latest:
print("Failed to check for the latest version")
return
current = re.sub(r'\.0+', '.', ipwb_version)
if latest == current:
print(f"Installed version {current} is up to date")
else:
print("A new update is available\nRun `pip install --upgrade ipwb` to upgrade.")
print(f"Latest version: {latest}\nInstalled version: {current}")
ipwb/util.py
Outdated
| return (currentVersion, latestVersion) | ||
| except Exception as e: | ||
| return (None, None) | ||
| latest_version = jResp['info']['version'] |
There was a problem hiding this comment.
It is possible that the returned JSON data is empty or has an error, in that case, this nested attribute access might throw another exception and the control main fail to reach to the next return statement.
There was a problem hiding this comment.
In this case, the exception will be handled by the caller, An unknown exception occurred. Perhaps we could/should handle it more directly. Thoughts?
There was a problem hiding this comment.
And that will be printed to the user something like:
An unknown error occurred.
Some ugly JSON parsing error message...
Which would be of no use of their as they cannot fix it. That's why I proposed a rather simpler approach to this simple task.
Co-authored-by: Sawood Alam <[email protected]>
Can you submit this as a code improvement amending this PR? I agree with your cohesive improvement of getting the latest version for potential reuse elsewhere. We will still need to handle the scenario when the request to pypi fails, which is the crux of this PR/issue, and is implemented using an exception model not present in your proposal. |
Sure!
I did not illustrate that in a verbose manner, because I said it this way: Which was an indication to reuse the code of loading and response parsing you already put in place. |
|
@github-actions run #! /bin/sh
export PS4="$ "
exec > comment.buffer 2>&1
echo "\`\`\`"
set -x
git checkout issue-700
pip install ./
ipwb -u
sed -i 's/2020/2019/' ipwb/__init__.py
pip install ./
ipwb -u
{ set +x; } 2>/dev/null
echo "\`\`\`" |
|
|
Well, the branch was deleted before I posted the above comment, but the code still worked. |
|
GitHub auto-deletes branches post-merge now. (even though it says that I deleted it) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes some handling of instances where pypi.org
is offline or inaccessible. I also cleaned up some of the
naming and introduced an exception dataclass to make handling
this issue more clear. Also also, I removed the dependence
on the six module to instead use the provided urllib
urlopen method.
Closes #700