Add config variable and check for platform warnings #6309
Conversation
|
Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack. For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide |
|
Thanks for helping make Bundler better! A couple of things that need to be done before we can merge this. This is going to need a test to make sure this is behaving as expected and so that we don't regress in the future, you will want to look at this test file. You will also need to document the new configuration option to the bundle config man page |
agrim123
force-pushed
the
disable_platform_warnings
branch
from
5b95686
to
94d399d
Compare
| it "prints a helpful warning when a dependency is unused on any platform" do | ||
| simulate_platform "ruby" | ||
| simulate_ruby_engine "ruby" | ||
| context "when disable_platform_warnings is false (by default)" do |
There was a problem hiding this comment.
This test doesn't need a context blocked wrapping around it as the setting is set in Bundler automatically.
| context "when disable_platform_warnings is true" do | ||
| before { bundle! "config disable_platform_warnings true" } | ||
|
|
||
| it "doesnot print the warning when a dependency is unused on any platform" do |
| bundle! "install" | ||
|
|
||
| expect(out).not_to include <<-O.strip | ||
| The dependency #{Gem::Dependency.new("rack", ">= 0")} will be unused by any of the platforms Bundler is installing for. Bundler is installing for ruby but the dependency is only for x86-mingw32, x86-mswin32, x64-mingw32, java. To add those platforms to the bundle, run `bundle lock --add-platform x86-mingw32 x86-mswin32 x64-mingw32 java`. |
There was a problem hiding this comment.
This is a lot to match exactly against in the output which could easily be missed if we change the test or behavior. I would match with:
expect(out).to_not include(/The dependency (.*) will be unused/)
There was a problem hiding this comment.
Cool. Will change it.
There was a problem hiding this comment.
However, using include is throwing a TypeError
TypeError:
no implicit conversion of Regexp into StringHow about using match?
There was a problem hiding this comment.
Yep, that was a typo on my part.
There was a problem hiding this comment.
Using
expect(out).not_to match(/The dependency (.*) will be unused/)does the trick. Is it good?
man/bundle-config.ronn
Outdated
| @@ -166,6 +166,8 @@ learn more about their operation in [bundle install(1)][bundle-install(1)]. | |||
| When set, Gemfiles containing multiple sources will produce errors | |||
| instead of warnings. | |||
| Use `bundle config --delete disable_multisource` to unset. | |||
| * `disable_platform_warnings` (`BUNDLE_DISABLE_PLATFORM_WARNINGS`): | |||
| Whether Bundler should show platform warnings. | |||
There was a problem hiding this comment.
I would reword this to be a bit more explicit, something like:
Disable warnings during bundle install when a dependency is unused on the current platform
|
Thanks! @bundlerbot r+ |
|
📌 Commit 5e73a89 has been approved by |
…ndale Add config variable and check for platform warnings Thanks so much for the contribution! To make reviewing this PR a bit easier, please fill out answers to the following questions. ### What was the end-user problem that led to this PR? The user needed a way to turn off platform warnings. ### What was your diagnosis of the problem? Creating a config variable to solve the above problem. ### What is your fix for the problem, implemented in this PR? Added a key `disable_platform_warnings` in settings and placed check at the relevant place to disable warnings. ### Why did you choose this fix out of the possible options? We will by default show warnings but the user might want to disable them, so using a config variable looked a good option. Fixes #6124
|
☀️ Test successful - status-travis |
…ndale Add config variable and check for platform warnings Thanks so much for the contribution! To make reviewing this PR a bit easier, please fill out answers to the following questions. ### What was the end-user problem that led to this PR? The user needed a way to turn off platform warnings. ### What was your diagnosis of the problem? Creating a config variable to solve the above problem. ### What is your fix for the problem, implemented in this PR? Added a key `disable_platform_warnings` in settings and placed check at the relevant place to disable warnings. ### Why did you choose this fix out of the possible options? We will by default show warnings but the user might want to disable them, so using a config variable looked a good option. Fixes #6124 (cherry picked from commit 23dfadc)
…ndale Add config variable and check for platform warnings Thanks so much for the contribution! To make reviewing this PR a bit easier, please fill out answers to the following questions. ### What was the end-user problem that led to this PR? The user needed a way to turn off platform warnings. ### What was your diagnosis of the problem? Creating a config variable to solve the above problem. ### What is your fix for the problem, implemented in this PR? Added a key `disable_platform_warnings` in settings and placed check at the relevant place to disable warnings. ### Why did you choose this fix out of the possible options? We will by default show warnings but the user might want to disable them, so using a config variable looked a good option. Fixes #6124 (cherry picked from commit 23dfadc)
Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.
What was the end-user problem that led to this PR?
The user needed a way to turn off platform warnings.
What was your diagnosis of the problem?
Creating a config variable to solve the above problem.
What is your fix for the problem, implemented in this PR?
Added a key
disable_platform_warningsin settings and placed check at the relevant place to disable warnings.Why did you choose this fix out of the possible options?
We will by default show warnings but the user might want to disable them, so using a config variable looked a good option.
Fixes #6124