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.
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
Adding update command #418
Adding update command #418
Changes from 9 commits
f6dba35
cac52b8
8ba8eb2
36d7826
bf5ee31
afc723b
1eb8236
cf0a1b3
fad531e
192bae9
163b3e3
a2dac59
2d5f2e9
eef3fb7
1fbafda
55f8647
62cf186
bf4f31e
9476e52
984c32e
b3f5356
cbffe4e
d79f1d2
f8670b6
72cb6f8
8647335
a1bf518
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably have enough GitHub interaction code to warrant a
GitHubClient
class. Then, ideally, I'd inject that into dependent classes via constructor parameters, and avoid instantiating new clients directly.DI is generally nice (for testing and dependency/behaviour tracking), and consistent with our general coding practice.
I'm willing to let this PR through without DI for now, but I would like us to refactor this soon-ish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use BmxException with better error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already a nested exception (not sure if we can un-nest it without making the block messy) so I don't think the user will see it. I think the "Failed to update with new files" message should capture this tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see. Didn't notice the nesting. I'd still like to avoid it, but it's good for now.