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

Call upgrade if something was passed to update #2858

Merged
merged 1 commit into from Jul 11, 2017

Conversation

Projects
None yet
5 participants
@marinintim
Copy link
Contributor

marinintim commented Jul 5, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

This pull request calls brew upgrade <formula> if was mistakenly called as brew update <formula>. If there is an obvious and single solution to user's mistake, why wouldn't homebrew fix it itself?

I didn't add any tests, because I'm not sure how to test this change.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Jul 5, 2017

upgrade and update are two different things, so as a user I wouldn't expect to be redirected to upgrade.

@marinintim

This comment has been minimized.

Copy link
Contributor

marinintim commented Jul 5, 2017

@reitermarkus: when you are aware of difference between upgrade и update, you would not call update with formula as argument anyway, as explained by error message. What's the case where user experiences this message and her next action is not to call brew upgrade with same arguments?

I use npm more often, where npm update is an alias to upgrade, and therefore I use update <pkg> automatically, so I had seen this message at least a few times.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 6, 2017

I use npm more often, where npm update is an alias to upgrade, and therefore I use update automatically, so I had seen this message at least a few times.

Thanks for the PR! I appreciate this explanation and if others feel the same way: please chime in on this PR but I'd like to hear from a lot more people before we add this.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 6, 2017

While it might be convenient to be able to use brew update foo and brew up foo, it seems like it would be surprising for brew update (without additional arguments) and brew upgrade (without additional arguments) to do wildly different things.

@vitorgalvao

This comment has been minimized.

Copy link
Member

vitorgalvao commented Jul 6, 2017

it seems like it would be surprising for brew update (without additional arguments) and brew upgrade (without additional arguments) to do wildly different things.

Agreed. It seems like someone would get used to update (because it’s always automatically corrected) and always use it, then be surprised when it acts differently from expected when used with no arguments.

Now that I think about it, since update is now called automatically, there’s little reason to ever call it on purpose. So if a user gets used to always calling upgrade instead, they also do not need to care about the difference and the issue will be solved.

@marinintim

This comment has been minimized.

Copy link
Contributor

marinintim commented Jul 7, 2017

Maybe then not call brew upgrade immediately, but instead print You probably meant 'brew upgrade whatever' instead of <formula>. Then it's easier to copy and paste.

But I do not see a problem that people would rely on this behaviour constantly and would be surprised when they'd try brew update without arguments. The error message is still here, notifying the user that this is not the intended usage of update, yet taking the most logical step immediately. This tidbit would improve UX of using brew a little without hurting anyone else.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 9, 2017

Maybe then not call brew upgrade immediately, but instead print You probably meant 'brew upgrade whatever' instead of . Then it's easier to copy and paste.

This is something we'd merge if you change it to output the full arguments for update.sh (not update-reset.sh). I'm now 👎 on making this run brew upgrade given the conversation above.

@marinintim

This comment has been minimized.

Copy link
Contributor

marinintim commented Jul 9, 2017

@MikeMcQuaid: I've updated pull request to only display message without running brew upgrade.

@@ -336,7 +336,8 @@ homebrew-update() {
*)
odie <<EOS
This command updates brew itself, and does not take formula names.
Use 'brew upgrade <formula>'.
Use 'brew upgrade <formula>' instead.
You probably want 'brew upgrade $@'...

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 10, 2017

Member

Use 'brew upgrade $@' instead. I think is sufficient and more concise, thanks.

This comment has been minimized.

@marinintim

@marinintim marinintim force-pushed the marinintim:update_to_upgrade branch 2 times, most recently from ee34788 to ea99a37 Jul 10, 2017

@marinintim

This comment has been minimized.

Copy link
Contributor

marinintim commented Jul 10, 2017

Rebased on current master.

Summary: after applying this change brew upgrade would not be called automagically, it only changes message to copy-paste ready one.

Display "Use `brew upgrade foo` instead" when calling update with args
So the user could just copy-paste the command.

@marinintim marinintim force-pushed the marinintim:update_to_upgrade branch from ea99a37 to 156e19c Jul 10, 2017

@MikeMcQuaid MikeMcQuaid merged commit db08eff into Homebrew:master Jul 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 11, 2017

Thanks for your first contribution to Homebrew, @marinintim! Without people like you submitting PRs we couldn't run this project. You rock!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

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