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

Document eval_gemfile #1081

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sambostock
Copy link

@sambostock sambostock commented Jan 21, 2023

Warning
This PR and rubygems/rubygems#6292 are mutually exclusive. At most one of them should be merged.
I personally think documenting it is the better approach, but I have opened rubygems/rubygems#6292 as the alternative.

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

eval_gemfile is a useful method used across many projects, but it is not explicitly documented. On the other hand, it isn't explicitly marked as undocumented via something like # :nodoc:. This leads to confusion as to whether it should be used or not, as in rails/rails#47033

What was your diagnosis of the problem?

We should clearly mark the method as part of the public API, or private API.

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

This adds documentation for eval_gemfile and how it can be used in one's Gemfile.

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

The alternative is to mark it as private using something a magic comment:

def eval_gemfile # :nodoc:

However, as this is in use by many projects, my personal preference is to see it become public. Searching GitHub for usage turns up examples such as the following:

This PR serves to start the discussion, so we can take a clear stance one way or the other. I've also opened rubygems/rubygems#6292, if we prefer to mark it as :nodoc: instead, although I personally would prefer to document it.

@sambostock
Copy link
Author

This comment was surfaced on the PR in Rails, but I'm not sure if it still applies, as it is from 2017, and it's possible things have changed since them (Bundler's stance, as well as community usage).

@simi
Copy link
Member

simi commented Jan 21, 2023

This comment was surfaced on the PR in Rails, but I'm not sure if it still applies, as it is from 2017, and it's possible things have changed since them (Bundler's stance, as well as community usage).

As I mentioned in Rails PR, I have asked other RubyGems maintainers and mainly @deivid-rodriguez mentioned it was never intended to be public API.

@ekohl
Copy link

ekohl commented Nov 28, 2023

For what it's worth, dependabot supports eval_gemfile but only with fixed strings. So you can't interpolate or use variables (such as glob bundler.d/*.rb). That does work just fine with bundler but a if this becomes a formal API, it would be nice to consider that use case too so third party tooling like dependabot can also support it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants