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

feature: include self-update status in check #2615

Merged
merged 1 commit into from Dec 28, 2020
Merged

feature: include self-update status in check #2615

merged 1 commit into from Dec 28, 2020

Conversation

hi-rustin
Copy link
Member

close #2577

@hi-rustin hi-rustin closed this Dec 19, 2020
@hi-rustin hi-rustin reopened this Dec 19, 2020
@hi-rustin
Copy link
Member Author

@kinnison Could you please take look? Thanks!

@kinnison
Copy link
Contributor

This looks pretty good. I'm not in a position to test this out today, but will endeavour to do so in the next few days.

kinnison
kinnison suggested changes Dec 22, 2020
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

In general I like this, though I think it'd be slightly better if rustup check contained a line for rustup in the case of there being no update. e.g.

rustup - Up to date : 1.23.1

Also, could you clean up your commit message when you're done? Currently it feels like you made some number of commits, then squashed them together without cleaning it up.

This is very close to being mergeable.

@hi-rustin hi-rustin force-pushed the rustin-patch-feat branch 4 times, most recently from c4a307f to 9842018 Compare December 22, 2020 10:16
@hi-rustin
Copy link
Member Author

In general I like this, though I think it'd be slightly better if rustup check contained a line for rustup in the case of there being no update. e.g.

Added.

@hi-rustin
Copy link
Member Author

hi-rustin commented Dec 22, 2020 •
edited

Thanks for your review! ❤️ @kinnison

kinnison
kinnison suggested changes Dec 23, 2020
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

Please would you add a test for the new message?

@hi-rustin
Copy link
Member Author

Please would you add a test for the new message?

Added.

kinnison
kinnison suggested changes Dec 24, 2020
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

Code content now looks good. Could you please fix up your commit message now. It is a history of everything you did and as such is not useful.

I'd suggest something along the lines of:

feature: include self-update status in check

Once the commit message is clean, I can merge this.

@hi-rustin hi-rustin changed the title Include rustup self-update status in check feature: include self-update status in check Dec 24, 2020
@hi-rustin
Copy link
Member Author

Code content now looks good. Could you please fix up your commit message now. It is a history of everything you did and as such is not useful.

I'd suggest something along the lines of:

feature: include self-update status in check

Once the commit message is clean, I can merge this.

Done, thanks for your review!

@hi-rustin
Copy link
Member Author

@kinnison ping~ 😄

@kinnison
Copy link
Contributor

Hi, there's still a large body in the commit message which reads like a history of the commit which isn't ideal. I've pushed a rebase which cleans that up for you, and assuming I didn't break anything and the CI goes green, I'll merge this. Thank you for your work on this feature.

@hi-rustin
Copy link
Member Author

Hi, there's still a large body in the commit message which reads like a history of the commit which isn't ideal. I've pushed a rebase which cleans that up for you, and assuming I didn't break anything and the CI goes green, I'll merge this. Thank you for your work on this feature.

Thanks for your review!

@kinnison kinnison merged commit 3c3a27f into rust-lang:master Dec 28, 2020
@hi-rustin hi-rustin deleted the rustin-patch-feat branch December 28, 2020 15:56
@ickk ickk mentioned this pull request Apr 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinnison kinnison kinnison 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.

Include rustup self-update status in rustup check
2 participants
@hi-rustin @kinnison