Switch PluginManager to use require_with_graceful_fail #4233

Merged
merged 1 commit into from Dec 8, 2015

Conversation

Projects
None yet
4 participants
@RochesterinNYC
Contributor

RochesterinNYC commented Dec 7, 2015

No description provided.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 7, 2015

Contributor

I'm not against this I'm against the wording, how does this improve the error message that Ruby by default uses? And if it does that's a bug in Ruby not in Jekyll.

Contributor

envygeeks commented Dec 7, 2015

I'm not against this I'm against the wording, how does this improve the error message that Ruby by default uses? And if it does that's a bug in Ruby not in Jekyll.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 7, 2015

Contributor

Maybe you also want to just switch it to Jekyll::External.require_with_graceful_fail?

Contributor

envygeeks commented Dec 7, 2015

Maybe you also want to just switch it to Jekyll::External.require_with_graceful_fail?

@RochesterinNYC

This comment has been minimized.

Show comment
Hide comment
@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

Okay, you're right, 1require_with_graceful_require1 looks like exactly the right error message we'd want. Updated the PR. Thanks for the point in the right direction!

Contributor

RochesterinNYC commented Dec 7, 2015

Okay, you're right, 1require_with_graceful_require1 looks like exactly the right error message we'd want. Updated the PR. Thanks for the point in the right direction!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 7, 2015

Contributor

@RochesterinNYC I would remove the "require gem" entirely and just just have that one single call, the graceful require does all that automatically, it's it's job.

Contributor

envygeeks commented Dec 7, 2015

@RochesterinNYC I would remove the "require gem" entirely and just just have that one single call, the graceful require does all that automatically, it's it's job.

@RochesterinNYC RochesterinNYC changed the title from Improve error message for LoadErrors caused by missing Gemfile plugins to Switch PluginManager#require_gems to use require_with_graceful_fail Dec 7, 2015

@RochesterinNYC

This comment has been minimized.

Show comment
Hide comment
@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

Ah, gotcha. require_with_graceful_fail handles the require itself. So it was just an artifact/missed refactor that require_gems wasn't using this method? Updated the PR.

Contributor

RochesterinNYC commented Dec 7, 2015

Ah, gotcha. require_with_graceful_fail handles the require itself. So it was just an artifact/missed refactor that require_gems wasn't using this method? Updated the PR.

@parkr

View changes

lib/jekyll/plugin_manager.rb
@@ -27,7 +27,7 @@ def require_gems
site.gems.each do |gem|
if plugin_allowed?(gem)
Jekyll.logger.debug("PluginManager:", "Requiring #{gem}")
- require gem
+ Jekyll::External.require_with_graceful_fail([gem])
end

This comment has been minimized.

@parkr

parkr Dec 7, 2015

Member

Could also improve this to be

Jekyll::External.require_with_graceful_fail(site.gems.select { |g| plugin_allowed?(g) })

What do you think?

@parkr

parkr Dec 7, 2015

Member

Could also improve this to be

Jekyll::External.require_with_graceful_fail(site.gems.select { |g| plugin_allowed?(g) })

What do you think?

This comment has been minimized.

@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

That looks like a cleaner change. If we want to one line this, is there still a point to having the require_gems method? It's only used on line 19 (PluginManager#conscientious_require) so we could just use this one-liner there? How do we feel about that? Something like:

    def conscientious_require
      require_plugin_files
      Jekyll::External.require_with_graceful_fail(site.gems.select { |g| plugin_allowed?(g) })
      deprecation_checks
    end
@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

That looks like a cleaner change. If we want to one line this, is there still a point to having the require_gems method? It's only used on line 19 (PluginManager#conscientious_require) so we could just use this one-liner there? How do we feel about that? Something like:

    def conscientious_require
      require_plugin_files
      Jekyll::External.require_with_graceful_fail(site.gems.select { |g| plugin_allowed?(g) })
      deprecation_checks
    end

This comment has been minimized.

@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

I also notice that require_plugin_files is still using require without the graceful_fail method.

@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

I also notice that require_plugin_files is still using require without the graceful_fail method.

This comment has been minimized.

@envygeeks

envygeeks Dec 7, 2015

Contributor

All this debate is a smell, that file needs to be reviewed.

@envygeeks

envygeeks Dec 7, 2015

Contributor

All this debate is a smell, that file needs to be reviewed.

This comment has been minimized.

@parkr

parkr Dec 7, 2015

Member

is there still a point to having the require_gems method?

Absolutely! Code clarity. This whole thing is for GitHub after all, for their security team to ensure that non-whitelisted gems are not required. So if we can encapsulate this as its own method, it's easier to audit (and to understand that line's purpose).

I also notice that require_plugin_files is still using require without the graceful_fail method.

Good point. You can fix that up too if you would like.

@parkr

parkr Dec 7, 2015

Member

is there still a point to having the require_gems method?

Absolutely! Code clarity. This whole thing is for GitHub after all, for their security team to ensure that non-whitelisted gems are not required. So if we can encapsulate this as its own method, it's easier to audit (and to understand that line's purpose).

I also notice that require_plugin_files is still using require without the graceful_fail method.

Good point. You can fix that up too if you would like.

This comment has been minimized.

@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

Okay, valid point, makes sense. I do notice that this would remove the Jekyll.logger.debug if we wanted to one-line it. Would this break backwards compatibility if anyone is asserting or relying on these debug messages?

Also, thanks for the quick feedback!

@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

Okay, valid point, makes sense. I do notice that this would remove the Jekyll.logger.debug if we wanted to one-line it. Would this break backwards compatibility if anyone is asserting or relying on these debug messages?

Also, thanks for the quick feedback!

This comment has been minimized.

@parkr

parkr Dec 7, 2015

Member

We make no guarantees, so they should not rely on it. Is there a debug message in require_with_graceful_fallback?

@parkr

parkr Dec 7, 2015

Member

We make no guarantees, so they should not rely on it. Is there a debug message in require_with_graceful_fallback?

This comment has been minimized.

@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

Nope, only a logger.error message about the Dependency Error in case of LoadError in require_with_graceful_fail.

Think it would be valuable to include a Jekyll.logger.debug message about which gem is being required in require_with_graceful_feedback, provided that we remove any redundant logger.debug messages in places that use require_with_graceful_fail. And looking through the codebase, there don't seem to be any.

@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

Nope, only a logger.error message about the Dependency Error in case of LoadError in require_with_graceful_fail.

Think it would be valuable to include a Jekyll.logger.debug message about which gem is being required in require_with_graceful_feedback, provided that we remove any redundant logger.debug messages in places that use require_with_graceful_fail. And looking through the codebase, there don't seem to be any.

This comment has been minimized.

@parkr

parkr Dec 7, 2015

Member

Make it so!

@parkr

parkr Dec 7, 2015

Member

Make it so!

@parkr parkr added the ux label Dec 7, 2015

@RochesterinNYC RochesterinNYC changed the title from Switch PluginManager#require_gems to use require_with_graceful_fail to Switch PluginManager to use require_with_graceful_fail Dec 7, 2015

@RochesterinNYC

This comment has been minimized.

Show comment
Hide comment
@RochesterinNYC

RochesterinNYC Dec 7, 2015

Contributor

Thanks for all the feedback. Updated the PR to use require_with_graceful_fail in both require methods in PluginManager and added a helpful debug log message to require_with_graceful_fail stating which plugin is being required.

Contributor

RochesterinNYC commented Dec 7, 2015

Thanks for all the feedback. Updated the PR to use require_with_graceful_fail in both require methods in PluginManager and added a helpful debug log message to require_with_graceful_fail stating which plugin is being required.

@parkr

View changes

lib/jekyll/plugin_manager.rb
- require f
- end
+ plugins_path.each do |plugin_search_path|
+ plugin_files = Dir[File.join(plugin_search_path, "**", "*.rb")]

This comment has been minimized.

@parkr

parkr Dec 8, 2015

Member

Let's do Utils.safe_glob(plugin_search_path, File.join("**", "*.rb")) here instead :)

@parkr

parkr Dec 8, 2015

Member

Let's do Utils.safe_glob(plugin_search_path, File.join("**", "*.rb")) here instead :)

This comment has been minimized.

@RochesterinNYC

RochesterinNYC Dec 8, 2015

Contributor

Sure, that sounds good. Is there still a need for using File.join if we're passing in explicit parameters for the call? (i.e. it'll always just return "**/*.rb"

@RochesterinNYC

RochesterinNYC Dec 8, 2015

Contributor

Sure, that sounds good. Is there still a need for using File.join if we're passing in explicit parameters for the call? (i.e. it'll always just return "**/*.rb"

This comment has been minimized.

@parkr

parkr Dec 8, 2015

Member

@RochesterinNYC Yes, the File.join returns "**/*.rb" on Linux and Mac, but will return "**\*.rb" on Windows. :)

@parkr

parkr Dec 8, 2015

Member

@RochesterinNYC Yes, the File.join returns "**/*.rb" on Linux and Mac, but will return "**\*.rb" on Windows. :)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 8, 2015

Member

@RochesterinNYC Thank you for being cool with all the back-and-forth. ❤️

Member

parkr commented Dec 8, 2015

@RochesterinNYC Thank you for being cool with all the back-and-forth. ❤️

Switch PluginManager to use require_with_graceful_fail
* Add debug statement specifying current plugin to External#require_with_graceful_fail
@RochesterinNYC

This comment has been minimized.

Show comment
Hide comment
@RochesterinNYC

RochesterinNYC Dec 8, 2015

Contributor

Updated the PR to use Utils#safe_glob. Thanks for all the helpful feedback!

Contributor

RochesterinNYC commented Dec 8, 2015

Updated the PR to use Utils#safe_glob. Thanks for all the helpful feedback!

parkr added a commit that referenced this pull request Dec 8, 2015

@parkr parkr merged commit 11959ab into jekyll:master Dec 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Dec 8, 2015

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 8, 2015

Member

Well done, @RochesterinNYC! 🎉 Thanks for your contribution.

Member

parkr commented Dec 8, 2015

Well done, @RochesterinNYC! 🎉 Thanks for your contribution.

@RochesterinNYC RochesterinNYC deleted the RochesterinNYC:patch-1 branch Dec 8, 2015

@RochesterinNYC

This comment has been minimized.

Show comment
Hide comment
@RochesterinNYC

RochesterinNYC Dec 8, 2015

Contributor

Thanks for the guidance and review!

Contributor

RochesterinNYC commented Dec 8, 2015

Thanks for the guidance and review!

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.