Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add force flag for update #101

Merged
merged 5 commits into from
Jun 23, 2021
Merged

add force flag for update #101

merged 5 commits into from
Jun 23, 2021

Conversation

sascha-andres
Copy link
Contributor

This PR suppresses the prompt asking to proceed with update, instead it goes straight to the update process

marcosnils
marcosnils reviewed May 25, 2021
View reviewed changes
Copy link
Owner

@marcosnils marcosnils left a comment

Choose a reason for hiding this comment

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

Thx for the contribution!

As a general comment, I don't think --force is the correct flag for this process. I think something like what apt does with --yes / -y is more convenient. Since force can be used for something different here.

cmd/update.go Outdated Show resolved Hide resolved
cmd/update.go Show resolved Hide resolved
marcosnils
marcosnils reviewed May 25, 2021
View reviewed changes
Copy link
Owner

@marcosnils marcosnils left a comment

Choose a reason for hiding this comment

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

Thx for the contribution!

As a general comment, I don't think --force is the correct flag for this process. I think something like what apt does with --yes / -y is more convenient. Since force can be used for something different here.

@sascha-andres
Copy link
Contributor Author

@marcosnils Any further comments?

sirlatrom
sirlatrom reviewed May 26, 2021
View reviewed changes
cmd/update.go Outdated Show resolved Hide resolved
@sascha-andres @sirlatrom
Update cmd/update.go
02c9e0d 
Co-authored-by: Sune Keller <sune.keller+github@gmail.com>
@sascha-andres
Copy link
Contributor Author

@sirlatrom Agreed, is a better wording

@marcosnils
Copy link
Owner

marcosnils commented May 26, 2021 •
edited
Loading

There still seem to be some formatting issues with the code. Did you run go fmt?
image

@sascha-andres
Copy link
Contributor Author

If I run go fmt nothing changes locally, so it seems the run was successful. My IDE is configured to run it on save.

@marcosnils
Copy link
Owner

marcosnils commented May 29, 2021 •
edited
Loading

If I run go fmt nothing changes locally, so it seems the run was successful. My IDE is configured to run it on save.

That's correct. Not sure why github diff is showing those green marks though 😕 . I thought it was because it's incorrectly indented but again, doesn't seem to be the case.

@sirlatrom WDYT?

@sirlatrom
Copy link
Collaborator

If I run go fmt nothing changes locally, so it seems the run was successful. My IDE is configured to run it on save.

That's correct. Not sure why github diff is showing those green marks though 😕 . I thought it was because it's incorrectly indented but again, doesn't seem to be the case.

@sirlatrom WDYT?

Sorry for not getting back to you earlier. LGTM!

@sirlatrom sirlatrom merged commit a638c4a into marcosnils:master Jun 23, 2021
@cristiand391 cristiand391 mentioned this pull request Jun 23, 2021
Closed
@cristiand391
Copy link
Contributor

Thank you @sascha-andres for working on this!

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

@sirlatrom sirlatrom sirlatrom left review comments

@marcosnils marcosnils marcosnils approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sascha-andres @marcosnils @sirlatrom @cristiand391