10000 cask: add statement when upgrade won't be installed by bevanjkay · Pull Request #13312 · Homebrew/brew · GitHub
[go: up one dir, main page]
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

cask: add statement when upgrade won't be installed #13312

Closed
wants to merge 1 commit into from
Closed

cask: add statement when upgrade won't be installed #13312

wants to merge 1 commit into from

Conversation

bevanjkay
Copy link
Member
@bevanjkay bevanjkay commented May 23, 2022 •
edited
Loading
  • 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 style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Follow up from #13275 to add a statement when the upgrade does not proceed due to the artifact having not changed.
At the moment, if the downloaded artifact has not changed, the upgrade does not proceed, but there is no output provided to the user to explain what has happened.

I'm not sure if this should be behind odebug or if the current implementation is correct.

CC: @apainintheneck

@BrewTestBot
Copy link
Member

Review period will end on 2022-05-24 at 05:44:08 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label May 23, 2022
@apainintheneck
Copy link
Contributor

The behavior for normal and version :latest casks is the same right now. Why do you think it should behave differently for version :latest casks?

@apainintheneck
Copy link
Contributor
apainintheneck commented May 23, 2022 •
edited
Loading

After looking around a bit, it seems like a message is displayed to the user when they try to upgrade a formula by name that is already up-to-date.

~ ❯❯❯ brew upgrade fish
Warning: fish 3.4.1 already installed

Nothing of the sort seems to exist for casks though regardless of the version.

@bevanjkay
Copy link
Member Author

That's a good point. And it could be worthwhile looking at where else similar statements should be included.

It may not be ideal to include this readout when running batch upgrades (brew upgrade --greedy) however I think it makes sense to include some kind of feedback when targeting a specific cask brew upgrade <token>

@MikeMcQuaid
Copy link
Member

Nothing of the sort seems to exist for casks though regardless of the version.

Given this: I think it should be probably added for all cases or left for none.

@bevanjkay
Copy link
Member Author

@MikeMcQuaid I'm happy to leave this for now and come back to it at a later date.

My concern here was that there was a change in command behaviour which has the potential to cause confusion (before I looked into it I had assumed the command had broken).

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid I'm happy to leave this for now and come back to it at a later date.

Fine with me (but stalebot may close it out if it's too long) 👍🏻

@bevanjkay bevanjkay closed this May 23, 2022
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label May 23, 2022
@bevanjkay bevanjkay mentioned this pull request May 24, 2022
Merged
7 tasks
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bevanjkay @BrewTestBot @apainintheneck @MikeMcQuaid
0