added exception handling for latest release checking and tests - #151
added exception handling for latest release checking and tests#151angus-yxz wants to merge 2 commits into
Conversation
VikramGoyal23
left a comment
There was a problem hiding this comment.
Hi! Thanks so much for your contribution! There's a couple of issues with the implementation, but it's a good step in the right direction!
|
@angus-yxz Thanks for your contribution and user feedback! Generally, the implementation is ok, however I think a better direction for us would be a global exception handler, rather than a specific scenario exception handler. We will consider this in future improvements of gitmastery. For this case, whether to allow users to continue after failure to check latest release, we can consider as well. Likely case is that offline users who cannot fetch latest release would not be able to use cc: @VikramGoyal23 |
Hi, yes, I agree with that approach, I didnt look too deeply into the source code implementation, and Im also not exactly sure why, but all the exercise download and verification worked on my end even when the gitmastery release check failed via that http get call. I dont think a single http release get failure should block the rest of the CLI from operating. In my case this patch was necessary for me to just use the CLI (otherwise I literally would be unable to continue with the rest of the lessons). I think the most idiomatic thing to do in this case is to log the failure and then continue attempting executing the gh commands and if those in turn fail then it is appropriate to raise and error out loudly rather than preemptively speculate and gate the rest of the application CLI functionality behind a single failed release check For the other maintainers, here is the full context behind the change: NUS-CS2103-AY2627-S1/forum#8 (comment) I am still encountering spurious connection closing on my end and other users have reported similar issues on particular networking setups, so this change is necessary just to access the functionality of the CLI since the gh commands still go through fine. Regardless, I think it is still a best practice to handle this single release check exception properly instead of letting it bubble and crash the entire application preventing usage of the rest of the functionality |
jovnc
left a comment
There was a problem hiding this comment.
Yup, agree with you that latest version check should not be in critical path, just some small code changes and should be ok to merge, thanks @angus-yxz !
c7d4ba0 to
0ed625d
Compare
|
accidentally pushed the commit with author attribution to my personal account (forgot to overwrite my global config) force pushed to change attribution back |
Gitmastery attempts to check for latest release via a requests get to github. This get has no exception handling so any connection errors either locally or remotely will cause the entire application to crash when the right thing to do is to handle and log the exception and continue on.
I added a small fix to handle this exception, logging a warning when the release check fails. Additionally, I added a timeout because on some networking configurations the CLI might just hang. I added a single test which sets env vars (that python's requests library respects https://requests.readthedocs.io/en/latest/user/advanced/#proxies) to mock a connection failure. Also ruff check shows formatting errors that have nothing to do with my own diffs, they were present in older commits so I just left those alone and formatted my own code to minimize the diff.
Also I know you asked in CONTRIBUTING.md for me to open an issue to ask to have this be assigned to me explicitly but its 5:30 in the morning right now and the change was small and focused enough to make on my own quickly to have a locally working binary so I didnt get stuck waiting.