Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bundle fund command #3390

Merged
merged 2 commits into from Oct 1, 2020
Merged

Conversation

gjtorikian
Copy link
Contributor

Migrated from rubygems/bundler#7668


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

This PR implements the bundle fund proposal in this accepted RFC.

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

The rationale and technical details for the bundle fund command are explained in the RFC, but roughly:

  • A new metadata key, funding_uri, can be specified, that points to a funding page for a gem
  • On a successful bundle install or bundle update, a message will be printed out that indicates gems which require support: 4 gems you depend on are looking for funding
  • bundle fund will list out all those gems, with their funding links
  • bundle info will include the funding_uri in its printout
  • You can restrict bundle fund to only list information for a specific group
  • Dependencies of dependencies are currently not listed in bundle fund; this is by design

@welcome
Copy link

welcome bot commented Mar 16, 2020

Thanks for opening a pull request and helping make RubyGems better! Someone from the RubyGems 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 Github Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Github Actions 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 #rubygems channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

@gjtorikian
Copy link
Contributor Author

Just bumping this to hope to get some attention to it. ✌️

@simi
Copy link
Member

simi commented Jun 7, 2020

Overall looks good to me. I have two suggestions:

  1. Once merged, promote funding_uri at https://guides.rubygems.org/specification-reference/#metadata as well.
  2. What about to make it part of bundle gem gemspec template?

@gjtorikian
Copy link
Contributor Author

1. Once merged, promote `funding_uri` at https://guides.rubygems.org/specification-reference/#metadata as well.

Sure, and individual rubygems.org/gems pages too.

2\. What about to make it part of `bundle gem` gemspec template?

From the RFC, I didn't want to do this. Maybe in a future iteration?

@gjtorikian
Copy link
Contributor Author

Hey, just bumping this...again.

@deivid-rodriguez
Copy link
Member

@gjtorikian I'm so sorry nobody stepped in to review this before. Thanks so much for being so patient.

I'm going to put this on my list for this week. If I haven't reviewed this by next Monday, please ping me again.

@deivid-rodriguez
Copy link
Member

@gjtorikian Could you rebase this PR, it's 1289 commits behind current HEAD, I'd like to make sure it's green before reviewing. I can also do that for you as well if you're busy.

@gjtorikian
Copy link
Contributor Author

@deivid-rodriguez Ok, I've rebased.

@deivid-rodriguez
Copy link
Member

Tests are failing. It's because we removed the bundle! test helper. You should use bundle now.

@gjtorikian
Copy link
Contributor Author

Ah, ok. Done!

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Sep 17, 2020

Hello @gjtorikian!

I just read the RFC, it looks really really good ❤️. I agree with everything, including the potentially most controversial parts like not adding an opt-out flag to skip the <n> gems you depend on are looking for funding message. It's just a small unintrusive line, I don't think people will be bothered by it.

Regarding the message, what do you think about tweaking it to <n> gems you **directly** depend on are looking for funding. That way we give some clue that they might not be the only code you depend on that it's looking for funding.

Regarding the functionality, it looks all good other than that little tweak. I'll go ahead with reviewing the implementation now.

@gjtorikian
Copy link
Contributor Author

@deivid-rodriguez That makes complete sense to me, and I've tweaked the message. I also removed the ! as it felt a bit too enthusiastic.

@gjtorikian gjtorikian force-pushed the go-fund-me branch 2 times, most recently from e5c684e to 5f586f1 Compare September 17, 2020 10:29
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a first pass. Looking good!

bundler/lib/bundler/cli/common.rb Show resolved Hide resolved
bundler/spec/update/gems/fund_spec.rb Outdated Show resolved Hide resolved
bundler/spec/update/gems/fund_spec.rb Outdated Show resolved Hide resolved
bundler/spec/install/gems/fund_spec.rb Outdated Show resolved Hide resolved
bundler/spec/install/gems/fund_spec.rb Outdated Show resolved Hide resolved
bundler/spec/install/gems/fund_spec.rb Outdated Show resolved Hide resolved
Copy link
Contributor Author

@gjtorikian gjtorikian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @deivid-rodriguez ! Everything made sense and I've made the changes.

bundler/lib/bundler/cli/common.rb Show resolved Hide resolved
bundler/spec/update/gems/fund_spec.rb Outdated Show resolved Hide resolved
bundler/spec/update/gems/fund_spec.rb Outdated Show resolved Hide resolved
@gjtorikian gjtorikian force-pushed the go-fund-me branch 2 times, most recently from 5dddf03 to 2efc5f3 Compare September 17, 2020 14:45
@gjtorikian
Copy link
Contributor Author

So many test issues 🙈! But all good now.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made another pass, mainly little style issues now.

bundler/lib/bundler/cli/common.rb Outdated Show resolved Hide resolved
bundler/lib/bundler/cli/common.rb Outdated Show resolved Hide resolved
bundler/lib/bundler/cli/fund.rb Show resolved Hide resolved
bundler/lib/bundler/cli/fund.rb Outdated Show resolved Hide resolved
bundler/spec/commands/fund_spec.rb Outdated Show resolved Hide resolved
bundler/spec/install/gems/fund_spec.rb Outdated Show resolved Hide resolved
bundler/spec/install/gems/fund_spec.rb Outdated Show resolved Hide resolved
bundler/spec/update/gems/fund_spec.rb Outdated Show resolved Hide resolved
@deivid-rodriguez
Copy link
Member

Ok @gjtorikian, sorry for the back and forth here.

The problem is that specs already does the filtering of excluding not configured groups, so we can't get the funding information for those from there. We could try resolve.materialize(current_dependencies) instead to get all materialized specs (without any group filtering), but in order for that to work, it turns out we'd need to actually install those specs, and that didn't happen because groups were filtered out in the first place.

So, since I don't see an easy fix for this, I propose we go back to #3390 (comment) and address that instead by further tweaking the message to

<n> installed gems you directly depend on are looking for funding.

to make it more clear that any gem exclusions happening before gem installation, for example without production, are not included in the count.

@deivid-rodriguez
Copy link
Member

@gjtorikian This looks good to me now! 🎉

Could you rebase and squash the PR into two commits (one for the post_install_message and tests, and one for the addition of bundle fund and tests)?

@gjtorikian
Copy link
Contributor Author

🎆

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

I'm aware that at this point I'm probably just being annoying, but technically the commit that needs requested_dependencies to be made public is the second, so the change making that method public belongs in there 😅.

Other than that, I'd like to thank you so much for your infinite patience and for your polite well-spaced pings along these past months 💜.

@gjtorikian
Copy link
Contributor Author

Heh, no worries. It's fixed up!

I am just excited to be able to add this and get some visibility for support. I have another change pending for visualizing this metadata on rubygems.org next. 😄

@deivid-rodriguez
Copy link
Member

Hei, that's awesome to hear. This will make a better ecosystem overall.

Regarding my last request, I don't think it's actually fixed because bundle fund requires dependencies_for to be made public, so that part should go with the first commit, while the post_install method requires requested_dependencies to be made public, so that part should be included in the second commit. The reason I ask for this is not only for a better git history, but also if I ever need to bisect something, I'd like each individual commit to pass specs individually.

@gjtorikian
Copy link
Contributor Author

The reason I ask for this is not only for a better git history, but also if I ever need to bisect something, I'd like each individual commit to pass specs individually

It's all good! It's your project, I'm just committing to it. 😆

Re-rebased and pushed.

@deivid-rodriguez
Copy link
Member

Awesome, thanks so much @gjtorikian!! 🎉

@deivid-rodriguez deivid-rodriguez merged commit ce27b98 into rubygems:master Oct 1, 2020
@simi
Copy link
Member

simi commented Oct 1, 2020

🥳

@gjtorikian
Copy link
Contributor Author

As a maintainer I know how problematic the next question is but I am compelled to ask: when can we expect a new release with these changes? 🙈

@deivid-rodriguez
Copy link
Member

We hope to release bundler 2.2.0.rc.2 next week including this feature.

deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020
Add `bundle fund` command

(cherry picked from commit ce27b98)
deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020
Add `bundle fund` command

(cherry picked from commit ce27b98)
deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020
Add `bundle fund` command

(cherry picked from commit ce27b98)
deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020
Add `bundle fund` command

(cherry picked from commit ce27b98)
deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020
Add `bundle fund` command

(cherry picked from commit ce27b98)
deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020
Add `bundle fund` command

(cherry picked from commit ce27b98)
EduardoGHdez added a commit to EduardoGHdez/ruby_jard that referenced this pull request Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants