-
Notifications
You must be signed in to change notification settings - Fork 1
feat: check for tool updates when printing current version #25
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
base: main
Are you sure you want to change the base?
Conversation
dido18
left a comment
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.
I would display a notification about a new version being available before every command, not only when running the version command.
The update check should be non-blocking — if an error occurs during the check, a general warning should be shown without preventing the command from running.
Co-authored-by: Per Tillisch <[email protected]>
.licenses/arduino-flasher-cli/go/github.com/arduino/go-paths-helper.dep.yml
Show resolved
Hide resolved
| type: go | ||
| summary: Package yaml implements YAML support for the Go language. | ||
| homepage: https://pkg.go.dev/gopkg.in/yaml.v3 | ||
| license: other |
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 strange; the license isn't discovered, but the CI passes anyway. We should verify that everithing works as expected
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.
I'm not sure on how licensee works. @per1234 is this expected?
| color.YellowString(i18n.Tr("A new release of Arduino Flasher CLI is available:")), | ||
| color.CyanString(currentVersion.String()), | ||
| color.CyanString(latestVersion.String()), | ||
| color.YellowString("https://www.arduino.cc/en/software/#flasher-tool")) |
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.
I would move the string concatenation into the caller not in this function.
IMHO, this function should return the latestVersion, and the message is built outside.
| } else { | ||
| bufferWarnings = append(bufferWarnings, msg) | ||
| } | ||
| logrus.Warning(msg) |
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.
I would remove logrus but I would keep json format
Motivation
Users should know if a new version of the tool is available.
Change description
A message is printed if a new version of the tool is available when checking the current version:
Additional Notes
Reviewer checklist
main.