-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Change default Caskroom path #21857
Change default Caskroom path #21857
Conversation
👏 Hooray! Over three years since #188 where we moved into |
@phinze Interesting to hear that was the original motivation and 🆒 that it's now resolved! |
All for this 👍 |
👍 |
Seems like we’re all in agreement and everything is correct. Thank you all. Merging. |
Ahhhh, I was just in the middle of typing my feedback :D |
@claui It’s still welcome! I did see your thumbs up before merging, so I hope there wasn’t a mistake that needed fixing. Also, on a tangential note to everyone, what about we get rid of the |
A few minor nitpicks:
and, regarding UX:
|
@vitorgalvao I couldn’t help but I was just too excited about it 😊 |
Well,
If they have unmoved apps, then This is still relatively close to #13966 to be considered part of its breaking changes (as mentioned in #13201). Fortunately, those are mostly done for now, and the small breaks we’re now discussing (#21372 and #21858) should affect just a handful of people. You are correct, thought, this is still a change that might warrant a bigger warning. Will set one in the main README. |
Fair enough.
You are correct. Personally, I’d like to avoid getting confused with multiple Caskrooms in my file system, especially while testing things and working on the core. As a stopgap measure, I have now decided to That said, I completely agree with you in that not many people are probably going to be affected. |
I was one of those surprised not to see my many installed casks anymore...luckily I found this issue... |
With regard to @claui's comment about UX and the "nothing to list" after the first brew upgrade. To be honest possibly scaring the shit out of the user making them think they may have lost all their apps (granted I was far more worried as I'd just run a cleanup, not just an update, thought perhaps cleanup has gone mad/rouge/insane and wiped everything) is probably not the best course of action. I understand @vitorgalvao's comments regarding brew cask list, but a lot of people use it frequently (in fact one of the most common methods I've seen for mass updating casks uses it) and haven't had any problems with it at all and just as many happily use the wonderful tool you're developing for us without ever visiting github and reading the repo's readme.. If ever there was a time to be informing users actually within the program to be "ignoring" disastrous sounding "error messages" this would be it. :) |
I've got a script to check for updated casks - the reason why I use Calling this an "UX issue" is an understatement. Leaving users scratching their heads trying to figure out out what happened and wasting time looking for how to fix it is not proper procedure. I can think of a few options that would/will avoid this:
Option 1. is the very minimum I'd expect but, aside from this thread, I can't even find it explicitly mentioned anywhere... |
This commit fixes an error occurring with both `install` and `install --force` where either would fail when encountering a broken symlink. Users _might_ run into that issue in the wake of PR #21857, and certainly _will_ be bitten by it once PR #21858 is merged. This commit changes the `install` case so it handles the condition gracefully. It also fixes `install --force` so before moving an app, it removes any broken symlinks which might stand in the way.
Locking this issue. Please direct any further comments after #21894 (comment). That comment addresses the change, why it happened, what will be done about it and a workaround for the meantime (it actually points to the comments that detail those). Apologies for the disturbance. |
Changes to the core
Closes #21603.
Pinging @caskroom/maintainers. I’d like to merge this as soon as possible, so it still kind of fits inside the breaking changes of #13966. Separated it in two commits,
core
anddocs
.