-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Conversation
I like this! Would it maybe make sense to justify all the different parts of the line as well? |
I think so. Something like below is what I'd like to see if possible. Removing all the repetitive labeling, removes clutter and makes it easier to read.
|
Only issue with that is that it will break any automated parsing of the |
Well, maybe we should be encouraging people to use |
@segiddins because it's not CSV formatted? It's possible. We can add the comma's in to help with that but it will require a little mod to their script. Alternatively, it could be a |
No, not that its not CSV, but rather because that header line has the same format as an outdated gem line |
If we make it an alternative option/something that needs to be specified via flags, I don't think it'll see much use. Ideally, it'd be nice to shore up and focus Alternatively, this could be a PR/change to Bundler 2.0? In which case, breaking the interface/format is acceptable? |
@segiddins Ah! I see. It's an easy thing to fix either do all caps or wrap them in parentheses or ... depends if we decide to go this route. @RochesterinNYC Agree, breaking changes like this is better done in major version upgrades. My thoughts on the flag option is it's ok to have low usage in the beginning as it will take time for awareness and adoption/user change. What's more important is to hear their feedback and see rate of adoption, though for a command utility it's harder than it is for a website. |
Is there any reason to keep the sorting based on the group, rather than the gem name? When I use |
Grouping by Gemfile groups, then by sub-dependencies (dependencies of the explicitly named dependencies in the Gemfile), is more logical than having an alphabetical list of gems as it can get confusing which type of gem dependency you're looking at. Additionally, from what I've seen in terms of |
I can only speak for myself, but I find the current output very confusing. The only times I've ever used I felt obligated to add my two cents because I put @tlynam up to this in the first place. I'll respect whatever decisions are made by the project maintainers and I promise I won't belabor the point. |
Could we support both use cases? What do you think if there were a --grouped option as a type of sorting? Do you think it could be confusing to read a list if there were several group combinations? Looking through our gemfile we have |
To be honest, I'm also confused by the way outdated prints an undifferentiated list that isn't in alphabetical order. I think might make sense if it did it in a hierarchical way, maybe like this:
But given the way that it prints now, I think alphabetizing the list would actually make a lot of sense. I also like the tabular output, and I think adding a To be fully backwards-compatible, we could alphabetize the current output, add |
Sounds good to me. 👍 |
I agree with that. |
We could keep the current output format if |
What do you think of this? This alphabetizes the current default. We already have a
|
Nice! I really like that. 👍 Check with @segiddins if he wishes to differentiate the title row for easy parsing as he raised a concern earlier about the title row being similar to a table row. |
This looks awesome @tlynam! |
☔ The latest upstream changes (presumably #4841) made this pull request unmergeable. Please resolve the merge conflicts. |
Welp. @tlynam - we just approved a PR to add grouping to this command (#4841) and left this one behind. :| I'm sure it was unintentional, lots of PRs and Issues and only so much time. That said, your proposed changes still seem relevant if you'd like to rebase this. FYI, I'm working on #5061 which is making some additional changes to it. (Big picture, I'd love to see this code cleaned up to better separate out the data gathering and presentation - it's all intertwined right now and makes it more difficult to extend for different views - though I won't be focusing on that). |
Thanks @chrismo, I'll work on improving this :) |
20fe0be
to
73fe587
Compare
I rewrote this, please let me know how it can be improved, thanks! :) |
thx @tlynam. if someone else can get to it before me, cool - imma having a very busy week that doesn't include any time for bundler :/ feel free to ping me or us on this PR if we neglect you too long, I don't mind the reminders :) |
Thanks @chrismo, I completely understand :) |
☔ The latest upstream changes (presumably #5114) made this pull request unmergeable. Please resolve the merge conflicts. |
With this, if we rename the hash, we don't have to change the indentation of every entry to keep it consistent.
The list part doesn't add much and it's very implementation dependent.
The output feels a bit cleaner because there's more space, and the term is more clear in my opinion.
I don't think it adds much to the output?
If bundler crashes, specs should fail anyways.
The "Latest" version may also be installed, so I think it's more accurate to use "Locked" here.
All the other screen output and checks for `--parseable` are doing inline, so let's do it here too.
`--parseable` checks `Bundler.ui.debug?` instead of look at the option directly.
2029182
to
76b1d89
Compare
I'll merge this in tomorrow if there's no further feedback. |
@bundlerbot r+ |
4474: Improve readability of outdated r=deivid-rodriguez a=tlynam Haven't fixed tests since wanted buy-in. Before: ``` Outdated gems included in the bundle: * active_model_serializers (newest 0.9.5, installed 0.8.3, requested ~> 0.8.0) in group "default" * activerecord-import (newest 0.13.0, installed 0.10.0) in group "default" * addressable (newest 2.4.0, installed 2.3.5, requested = 2.3.5) in group "default" * airbrake (newest 5.2.3, installed 5.1.0) in group "default" * bootstrap-sass (newest 3.3.6, installed 3.3.5.1) in group "default" * capistrano (newest 3.5.0, installed 3.4.0, requested ~> 3.4) in group "development" * capistrano-rails (newest 1.1.6, installed 1.1.3, requested ~> 1.1.1) in group "development" ``` After: ``` --pretty Outdated gems included in the bundle: Gem Name New Installed Requested Groups airbrake 5.3.0 5.0.3 default better_errors 2.1.1 1.0.1 ~> 1.0.1 development bootstrap-sass 3.3.6 3.3.0.1 ~> 3.3.0.1 default factory_girl_rails 4.7.0 4.4.1 test, development --pretty --verbose Outdated gems included in the bundle: Gem Name New Installed Requested Groups Load Path airbrake 5.3.0 5.0.3 default /Users/todd/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/specifications/airbrake-5.3.0.gemspec better_errors 2.1.1 1.0.1 ~> 1.0.1 development /Users/todd/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/specifications/better_errors-2.1.1.gemspec bootstrap-sass 3.3.6 3.3.0.1 ~> 3.3.0.1 default /Users/todd/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/specifications/bootstrap-sass-3.3.6.gemspec factory_girl 4.7.0 4.4.0 /Users/todd/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/specifications/factory_girl-4.7.0.gemspec factory_girl_rails 4.7.0 4.4.1 test, development /Users/todd/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/specifications/factory_girl_rails-4.7.0.gemspec ``` Co-authored-by: Todd Lynam <[email protected]> Co-authored-by: David Rodríguez <[email protected]>
Build succeeded |
Haven't fixed tests since wanted buy-in.
Before:
After: