Warn developers when uninstalling a dependency #1498
Conversation
| msg << " currently installed." | ||
| msg << "\nYou can silence this warning with " | ||
| msg << "`brew uninstall --ignore-dependencies " | ||
| msg << "#{requireds.map(&:name).join(" ")}`." |
There was a problem hiding this comment.
Will merge this as-is but would be nice to refactor this to use EOS.undent and a bunch of variables declared before instead. Similarly below.
| @@ -376,6 +376,14 @@ def ignore_interrupts(opt = nil) | |||
| trap("INT", std_trap) | |||
| end | |||
|
|
|||
| def capture_stderr | |||
There was a problem hiding this comment.
Think you can use shutup here. Again, won't let it block this PR but for post-merge refactoring that'd be good if it's possible.
There was a problem hiding this comment.
I don't think shutup returns the captured output. Would you be open to it being refactored to return the captured stdout and stderr?
There was a problem hiding this comment.
Yeh, consolidation there makes sense although might want to tweak the name if we do that too. Definitely one for another PR 👍
| @@ -376,6 +376,14 @@ def ignore_interrupts(opt = nil) | |||
| trap("INT", std_trap) | |||
| end | |||
|
|
|||
| def capture_stderr | |||
| old, $stderr = $stderr, StringIO.new | |||
There was a problem hiding this comment.
brew style complains about the double assignment.
|
Unfortunately still some (different) failing tests after rerunning. |
|
Will aim to get this fixed today so you can merge. |
Suggested in Homebrew#1084. Made the existing warning output entirely to STDERR, because previously the first line went to STDERR and subsequent ones went to STDOUT.
alyssais
force-pushed
the
uninstall_developer_warning
branch
from
a0d396f
to
3c310b2
Compare
|
Thanks @alyssais! |
|
Looks like the failures went away after rebased. Thought CI would merge into latest master, but I guess not? |
|
It does normally. Maybe Something Weird Happened. |
| msg << "`brew uninstall --ignore-dependencies " | ||
| msg << "#{requireds.map(&:name).join(" ")}`." | ||
| opoo msg | ||
| class DependentsMessage |
There was a problem hiding this comment.
Put all the shared stuff between the two messages into a class so it can be re-used between both of them. It's a bit longer, but I think much cleaner now.
|
Can we have an environment variable to make these warnings go away? |
|
@ilovezfs could do. What's the motivation? |
|
To not have a regression where I'm spammed with warnings. |
|
@ilovezfs If you're getting spammed with warnings where you shouldn't be, should we not be fixing that rather than hiding it under an environment variable? We've already had one bug (#1496) that went undetected for longer than it should have because developers had different functionality than non-developers, and I'd like to stop something like that happening again. Can you clarify if there's an actual regression where Homebrew is warning you where it shouldn't be, or if you just consider the fact that there's a warning in the first place to be a regression? |
this. |
Works around Rubycop not liking method names that start with `is_` by changing convention from singular to plural. I think it's better that way anyway.
|
That |
No. |
|
Because ... |
I'd like to see far more than one person request such a thing before we add it.
This is a good argument. |
|
The original feature was said to be disabled for developers. This makes it no longer disabled, and annoying in a novel way. |
|
Quoting myself:
|
|
Not cool. |
|
Yep, I don't think the |
|
@alyssais Have trigged a rebuild. CC |
|
@alyssais 💚 Will merge later, nice work! |
brew testswith your changes locally? Only with keg: don't rely on #to_formula #1497 merged.Suggested by @MikeMcQuaid in #1082 (comment).
Tests will fail as a side effect of #1496. When #1497 is merged, run the CI here again. It should go green and this should then be ready to merge.
I fixed the existing error message for non-developers as part of this too, so that it's output entirely to
stderr. Previously, the first line was sent tostderrand subsequent ones went tostdout.