Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Add missing bundle info feature, bug fix, and specs #7026

Merged
10 commits merged into from
Mar 31, 2019
Merged

Add missing bundle info feature, bug fix, and specs #7026

10 commits merged into from
Mar 31, 2019

Conversation

deivid-rodriguez
Copy link
Member

What was the end-user problem that led to this PR?

The problem was that bundler info GEM did not have feature parity with bundle show GEM, and it should because it's the recommended alternative. Also, bundle info bundler was showing an incorrect path while bundle show bundler was correct.

What was your diagnosis of the problem?

My diagnosis was that some code should be pulled from bundle show.

What is your fix for the problem, implemented in this PR?

My fix is to port the missing feature, bug fix, and specs to bundle info from bundle show.

Why did you choose this fix out of the possible options?

I chose this fix because it guarantees that the recommended alternative will work similarly to the current show command.

@deivid-rodriguez
Copy link
Member Author

I just noticed that bundle show GEM accepts a regexp, but bundle info GEM does not. Should I bring that feature too?

$ bundle show rail
1 : rails-dom-testing
2 : rails-html-sanitizer
3 : railties
4 : coffee-rails
5 : jquery-rails
6 : autoprefixer-rails
7 : capistrano-rails
8 : dotenv-rails
9 : factory_bot_rails
10 : rails-i18n
11 : sprockets-rails
12 : rails
13 : rspec-rails
14 : sass-rails
15 : slim-rails
16 : unicorn-rails
0 : - exit -

vs

$ bundle info rail
Could not find gem 'rail'.
Did you mean rails?

@colby-swandale
Copy link
Member

That's bundle list

@deivid-rodriguez
Copy link
Member Author

Oh, nice! We should document that bundle list feature since it's currently undocumented.

Then I'll remove the spec about the regexp matching, since the feature is not really there and it's only passing by chance... 😅

@deivid-rodriguez
Copy link
Member Author

Actually, this is not true. The new bundle list only applies to sets of gems:

$ RUBYOPT=-I/home/deivid/Code/bundler/lib ../bundler/bin/bundle list rails
ERROR: "bundle list" was called with arguments ["rails"]
Usage: "bundle list"

$ RUBYOPT=-I/home/deivid/Code/bundler/lib ../bundler/bin/bundle list rail
ERROR: "bundle list" was called with arguments ["rail"]
Usage: "bundle list"

I think we can bring this feature from bundle show too, so that the alternative works as much as possible in the same way as the deprecated command.

For consistency with `bundle show` specs.
This case `bundle info rails` is already tested at the beginning of the
file. Plus, the description is misleading since no `--path` option is
given here.
To be just like how current `bundle show` is tested.
Skip for now the functionality not currently supported in `bundle info`.
Just like `bundle show rails` did.
The raise is using the name of the class on its message!

```
Could not find gem 'Bundler::CLI::Common'.
```

Since we've never received a WTF report about this, I'll assume this is
just dead code and remove it.
@deivid-rodriguez
Copy link
Member Author

I ported the missing feature from bundle show. Now bundle info has feature parity for the use cases of the current bundle show acting on a specific argument.

Something I also noticed is that bundle info works against any default gem while bundle show does not. In my opinion, bundle show is correct here. bundle info or bundle show should only give information about the gems being currently included in the bundle. If the user is explicitly specifying a default gem in the gemfile, then yes, bundle info or bundle show should report it, possibly with a note that it's a default gem.

If a user wants to see information about any installed gem, not necessarily in the bundle, there's gem info already.

Thoughts about this?

@deivid-rodriguez
Copy link
Member Author

@indirect Do you have any opinion about this? I'm torn here. My previous message kind of makes sense to me and might be strictly more correct, but in real life would probably lead to worse behavior. As of today:

$ bundle info zlib
  * zlib (1.0.0)
	Summary: Ruby interface for the zlib compression/decompression library
	Homepage: https://github.com/ruby/zlib
	Path: /home/deivid/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/zlib-1.0.0
	Default Gem: yes
$ bundle show zlib
Could not find gem 'zlib'.

Regardless of this, this PR should be ready to go!

@indirect
Copy link
Member

@deivid-rodriguez oh man, you're right. bundle info should only work on gems that are inside the bundle, like bundle ls should only show gems that are inside the bundle. I guess that's a bug. :)

Thanks for getting this fixed up!

@bundlerbot r+

ghost pushed a commit that referenced this pull request Mar 31, 2019
7026: Add missing `bundle info` feature, bug fix, and specs r=indirect a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem was that `bundler info GEM` did not have feature parity with `bundle show GEM`, and it should because it's the recommended alternative. Also, `bundle info bundler` was showing an incorrect path while `bundle show bundler` was correct.

### What was your diagnosis of the problem?

My diagnosis was that some code should be pulled from `bundle show`.

### What is your fix for the problem, implemented in this PR?

My fix is to port the missing feature, bug fix, and specs to `bundle info` from `bundle show`.

### Why did you choose this fix out of the possible options?

I chose this fix because it guarantees that the recommended alternative will work similarly to the current show command.

Co-authored-by: David Rodríguez <[email protected]>
@ghost
Copy link

ghost commented Mar 31, 2019

Build succeeded

@ghost ghost merged commit 83d22e7 into master Mar 31, 2019
@ghost ghost deleted the new_info_specs branch March 31, 2019 02:04
@colby-swandale colby-swandale added this to the 2.1.0 milestone Apr 4, 2019
@deivid-rodriguez deivid-rodriguez removed this from the 2.1.0 milestone Dec 13, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants