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

Discover external commands through Bundler #910

Merged
merged 1 commit into from Jul 24, 2016
Merged

Discover external commands through Bundler #910

merged 1 commit into from Jul 24, 2016

Conversation

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 13, 2016

Other gems may want to add commands to the nanoc CLI. To do this
they need to run Nanoc::CLI.add_after_setup_proc. Currently, there
is no way to hook into the CLI. This commit adds such a way by
utilizing Bundler. E.g. the gem guard-nanoc could introduce new
commands when added to the Gemfile in this way:

gem 'nanoc'
group :nanoc do
  gem 'guard-nanoc'
end

Presence of any gems in the group :nanoc will cause the library with
the same name as the gem to be required by Bundler during
the initialization of the CLI.

A special group :nanoc is used for two reasons:

  • nanoc may not be the only or primary user of the Gemfile,
    and requiring many unrelated gems will slow down every invocation
    of nanoc with no workaround;
  • existing Gemfiles may include specifications for gems that cannot
    be successfully required, so using the group :default would
    break such existing nanoc installations.
@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Jul 13, 2016

Looks good!

The group name nanoc doesn’t convey the idea that these are gems that will be automatically required. Perhaps a group namesautorequire?

Additionally, since Bundler is not a runtime requirement of Nanoc, Bundler.require(:nanoc) can potentially fail.

Loading

Other gems may want to add commands to the nanoc CLI. To do this
they need to run Nanoc::CLI.add_after_setup_proc. Currently, there
is no way to hook into the CLI. This commit adds such a way by
utilizing Bundler. E.g. the gem guard-nanoc could introduce new
commands when added to the Gemfile in this way:

  gem 'nanoc'
  group :nanoc do
    gem 'guard-nanoc'
  end

Presence of any gems in the group :nanoc will cause the library with
the same name as the gem to be required by Bundler during
the initialization of the CLI.

A special group :nanoc is used for two reasons:
  * nanoc may not be the only or primary user of the Gemfile,
    and requiring many unrelated gems will slow down every invocation
    of nanoc with no workaround;
  * existing Gemfiles may include specifications for gems that cannot
    be successfully required, so using the group :default would
    break such existing nanoc installations.
@whitequark
Copy link
Member Author

@whitequark whitequark commented Jul 13, 2016

The group name nanoc doesn’t convey the idea that these are gems that will be automatically required. Perhaps a group named autorequire?

I'm not sure if such change would make a lot of sense. First, there is no guarantee that the entire Gemfile would not be autorequired; indeed, whether or not to require gems is a choice of an application using the Gemfile. Second, autorequiring groups of gems is, as far as I'm aware, literally the purpose of the Bundler group feature (e.g. see its docs), so that should not come as a surprise.

Additionally, since Bundler is not a runtime requirement of Nanoc, Bundler.require(:nanoc) can potentially fail.

I've wrapped that in if defined?(Bundler).

Loading

@ddfreyne ddfreyne added this to the 4.3 milestone Jul 13, 2016
@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Jul 13, 2016

The group name nanoc doesn’t convey the idea that these are gems that will be automatically required. Perhaps a group named autorequire?

I'm not sure if such change would make a lot of sense. First, there is no guarantee that the entire Gemfile would not be autorequired; indeed, whether or not to require gems is a choice of an application using the Gemfile. Second, autorequiring groups of gems is, as far as I'm aware, literally the purpose of the Bundler group feature (e.g. see its docs), so that should not come as a surprise.

Thinking about it again, the group name nanoc makes sense (as in, these are the gems that nanoc will load by default).

Loading


if defined?(Bundler)
# Discover external commands through Bundler
Bundler.require(:nanoc)
Copy link
Member

@ddfreyne ddfreyne Jul 13, 2016

Choose a reason for hiding this comment

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

What happens if no nanoc group exists? What if it is empty?

A change without tests puts me off, but at the same time, I have no idea how to test this in an automated fashion.

Loading

Copy link
Member Author

@whitequark whitequark Jul 13, 2016

Choose a reason for hiding this comment

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

Nothing. That's how this Bundler API works.

Loading

@ddfreyne ddfreyne merged commit d573212 into nanoc:master Jul 24, 2016
1 check passed
Loading
@whitequark whitequark deleted the bundler-require branch Jul 24, 2016
@whitequark
Copy link
Member Author

@whitequark whitequark commented Jul 24, 2016

Thanks!

Loading

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

Successfully merging this pull request may close these issues.

None yet

2 participants