This repository has been archived by the owner on Apr 14, 2021. It is now read-only.
skip colorize examples when the environment was mswin or non tty. #6994
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@hsbt I think this is related to the fix @colby-swandale and I worked on in #6957. Can you double check whether 577b522 fixes this issue? |
|
It's related. I'm not sure that 577b522 is the correct approach. Because |
|
I think both fixes are correct. Ours is preventing the thor's shell to be memoized at the class level, and thus preventing our specs from using the right shell for what they are testing. Yours is acknowledging that a colorized shell is not supported on Windows, trusting |
deivid-rodriguez
approved these changes
Feb 28, 2019
|
@bundlerbot r+ |
ghost
pushed a commit
that referenced
this pull request
Mar 6, 2019
6994: skip colorize examples when the environment was mswin or non tty. r=hsbt a=hsbt ### What was the end-user problem that led to this PR? Fixes #6900 ### What was your diagnosis of the problem? The current bundler examples were not supported on mswin and non-tty environments. ### What is your fix for the problem, implemented in this PR? I make to skip them when Thor detected unsupported environment. ### Why did you choose this fix out of the possible options? Thor is vendoered now. It's easy to fix bundler's example directly. Co-authored-by: SHIBATA Hiroshi <hsbt@ruby-lang.org>
Build succeeded |
ghost
merged commit 
ghost
pushed a commit
that referenced
this pull request
Nov 1, 2019
7401: Stop silencing output by default r=indirect a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that bundler defaults to a silent UI: https://github.com/bundler/bundler/blob/e70643c1be3a4417bd537d7e63470265465e693e/lib/bundler.rb#L66-L68 In my opinion, this isn't a good behavior for a CLI tool, and forces us to override it in many many different places. It has also caused several issues, for example, #3710 where `bundle list` was printing nothing. The [solution to that issues](#3707) led us to add yet more places where we override the default UI, and @indirect [predicting that having to unset the UI everytime we want it to not be silent](#3707 (comment)) would cause many headaches. Well, yeah... I've lost a lot of time trying to figure out why UI was silent sometimes, and normal another times, why some specs printed warnings and some didn't. In particular, see my series of "big fail PRs" fighting against bundler's UI: #7284, #7294, #7305, #7362. Another series of issues/PRs probably related to this is issue #6900, where the output would use a different UI on different environments. We had a lot of trouble to reliably fix it (#6994, #7002, #7253). I also run into these issues again when trying out the `RUBYGEMS_GEMDEPS` environment variable that enables `bundler/setup` from rubygems. ### What was your diagnosis of the problem? My diagnosis was that we shouldn't silence UI by default. ### What is your fix for the problem, implemented in this PR? My fix is to, instead of silencing and then overriding the default shell at many places, don't silence it by default and instead make it silent when needed. By doing this, I managed to get 100% of our specs green, so I'm pretty confident that the output is still the same (or if it's not, it's probably because some output/errors where being unintentionally swallowed). Now specs should pass, but they print a bunch of output to the screen. You can see error messages, hard crashes, success messages... Some of them might be showing actual issues with either the code or tests, so I plan to go through each of them and review them. I can do that in this PR or separately, no strong opinion. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
ghost
pushed a commit
that referenced
this pull request
Nov 4, 2019
7401: Stop silencing output by default r=indirect a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that bundler defaults to a silent UI: https://github.com/bundler/bundler/blob/e70643c1be3a4417bd537d7e63470265465e693e/lib/bundler.rb#L66-L68 In my opinion, this isn't a good behavior for a CLI tool, and forces us to override it in many many different places. It has also caused several issues, for example, #3710 where `bundle list` was printing nothing. The [solution to that issues](#3707) led us to add yet more places where we override the default UI, and @indirect [predicting that having to unset the UI everytime we want it to not be silent](#3707 (comment)) would cause many headaches. Well, yeah... I've lost a lot of time trying to figure out why UI was silent sometimes, and normal another times, why some specs printed warnings and some didn't. In particular, see my series of "big fail PRs" fighting against bundler's UI: #7284, #7294, #7305, #7362. Another series of issues/PRs probably related to this is issue #6900, where the output would use a different UI on different environments. We had a lot of trouble to reliably fix it (#6994, #7002, #7253). I also run into these issues again when trying out the `RUBYGEMS_GEMDEPS` environment variable that enables `bundler/setup` from rubygems. ### What was your diagnosis of the problem? My diagnosis was that we shouldn't silence UI by default. ### What is your fix for the problem, implemented in this PR? My fix is to, instead of silencing and then overriding the default shell at many places, don't silence it by default and instead make it silent when needed. By doing this, I managed to get 100% of our specs green, so I'm pretty confident that the output is still the same (or if it's not, it's probably because some output/errors where being unintentionally swallowed). Now specs should pass, but they print a bunch of output to the screen. You can see error messages, hard crashes, success messages... Some of them might be showing actual issues with either the code or tests, so I plan to go through each of them and review them. I can do that in this PR or separately, no strong opinion. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
deivid-rodriguez
pushed a commit
that referenced
this pull request
Nov 7, 2019
7401: Stop silencing output by default r=indirect a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that bundler defaults to a silent UI: https://github.com/bundler/bundler/blob/e70643c1be3a4417bd537d7e63470265465e693e/lib/bundler.rb#L66-L68 In my opinion, this isn't a good behavior for a CLI tool, and forces us to override it in many many different places. It has also caused several issues, for example, #3710 where `bundle list` was printing nothing. The [solution to that issues](#3707) led us to add yet more places where we override the default UI, and @indirect [predicting that having to unset the UI everytime we want it to not be silent](#3707 (comment)) would cause many headaches. Well, yeah... I've lost a lot of time trying to figure out why UI was silent sometimes, and normal another times, why some specs printed warnings and some didn't. In particular, see my series of "big fail PRs" fighting against bundler's UI: #7284, #7294, #7305, #7362. Another series of issues/PRs probably related to this is issue #6900, where the output would use a different UI on different environments. We had a lot of trouble to reliably fix it (#6994, #7002, #7253). I also run into these issues again when trying out the `RUBYGEMS_GEMDEPS` environment variable that enables `bundler/setup` from rubygems. ### What was your diagnosis of the problem? My diagnosis was that we shouldn't silence UI by default. ### What is your fix for the problem, implemented in this PR? My fix is to, instead of silencing and then overriding the default shell at many places, don't silence it by default and instead make it silent when needed. By doing this, I managed to get 100% of our specs green, so I'm pretty confident that the output is still the same (or if it's not, it's probably because some output/errors where being unintentionally swallowed). Now specs should pass, but they print a bunch of output to the screen. You can see error messages, hard crashes, success messages... Some of them might be showing actual issues with either the code or tests, so I plan to go through each of them and review them. I can do that in this PR or separately, no strong opinion. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net> (cherry picked from commit 1a585f5)
ghost
pushed a commit
to rubygems/rubygems
that referenced
this pull request
Mar 11, 2020
7401: Stop silencing output by default r=indirect a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that bundler defaults to a silent UI: https://github.com/bundler/bundler/blob/e70643c1be3a4417bd537d7e63470265465e693e/lib/bundler.rb#L66-L68 In my opinion, this isn't a good behavior for a CLI tool, and forces us to override it in many many different places. It has also caused several issues, for example, rubygems/bundler#3710 where `bundle list` was printing nothing. The [solution to that issues](rubygems/bundler#3707) led us to add yet more places where we override the default UI, and @indirect [predicting that having to unset the UI everytime we want it to not be silent](rubygems/bundler#3707 (comment)) would cause many headaches. Well, yeah... I've lost a lot of time trying to figure out why UI was silent sometimes, and normal another times, why some specs printed warnings and some didn't. In particular, see my series of "big fail PRs" fighting against bundler's UI: rubygems/bundler#7284, rubygems/bundler#7294, rubygems/bundler#7305, rubygems/bundler#7362. Another series of issues/PRs probably related to this is issue rubygems/bundler#6900, where the output would use a different UI on different environments. We had a lot of trouble to reliably fix it (rubygems/bundler#6994, rubygems/bundler#7002, rubygems/bundler#7253). I also run into these issues again when trying out the `RUBYGEMS_GEMDEPS` environment variable that enables `bundler/setup` from rubygems. ### What was your diagnosis of the problem? My diagnosis was that we shouldn't silence UI by default. ### What is your fix for the problem, implemented in this PR? My fix is to, instead of silencing and then overriding the default shell at many places, don't silence it by default and instead make it silent when needed. By doing this, I managed to get 100% of our specs green, so I'm pretty confident that the output is still the same (or if it's not, it's probably because some output/errors where being unintentionally swallowed). Now specs should pass, but they print a bunch of output to the screen. You can see error messages, hard crashes, success messages... Some of them might be showing actual issues with either the code or tests, so I plan to go through each of them and review them. I can do that in this PR or separately, no strong opinion. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was the end-user problem that led to this PR?
Fixes #6900
What was your diagnosis of the problem?
The current bundler examples were not supported on mswin and non-tty environments.
What is your fix for the problem, implemented in this PR?
I make to skip them when Thor detected unsupported environment.
Why did you choose this fix out of the possible options?
Thor is vendoered now. It's easy to fix bundler's example directly.