Skip to content

added exception handling for latest release checking and tests - #151

Open
angus-yxz wants to merge 2 commits into
git-mastery:mainfrom
angus-yxz:fix/connection-error-on-release-check
Open

added exception handling for latest release checking and tests#151
angus-yxz wants to merge 2 commits into
git-mastery:mainfrom
angus-yxz:fix/connection-error-on-release-check

Conversation

@angus-yxz

Copy link
Copy Markdown

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.

@VikramGoyal23 VikramGoyal23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment thread app/cli.py
Comment thread app/cli.py Outdated
@jovnc

jovnc commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

@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 gitmastery as well, as our exercises download and verification requires internet connection.

cc: @VikramGoyal23

@angus-yxz

angus-yxz commented Aug 13, 2026

Copy link
Copy Markdown
Author

@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 gitmastery as well, as our exercises download and verification requires internet connection.

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 jovnc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !

Comment thread tests/e2e/test_version.py
Comment thread app/cli.py Outdated
Comment thread app/cli.py Outdated
@angus-yxz

Copy link
Copy Markdown
Author

accidentally pushed the commit with author attribution to my personal account (forgot to overwrite my global config) force pushed to change attribution back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants