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

Rubocop theme and url jekyll libs #4959

Merged
merged 3 commits into from Jun 3, 2016

Conversation

Projects
None yet
4 participants
@derekgottlieb
Contributor

derekgottlieb commented May 28, 2016

Quick pass at trying to get lib/jekyll/theme and lib/jekyll/url passing rubocop for #4885

@derekgottlieb derekgottlieb referenced this pull request May 28, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
Show outdated Hide outdated lib/jekyll/theme.rb
@@ -38,7 +38,7 @@ def path_for(folder)
return unless resolved_dir
path = Jekyll.sanitized_path(root, resolved_dir)
path if Dir.exists?(path)
path if Dir.exist?(path)

This comment has been minimized.

@envygeeks

envygeeks May 28, 2016

Contributor

Use File.directory? it's straight to the point. Dir.exist? actually points to File.directory?

@envygeeks

envygeeks May 28, 2016

Contributor

Use File.directory? it's straight to the point. Dir.exist? actually points to File.directory?

# Remove leading '/' to avoid generating urls with `//`
result.gsub(/\/:#{token.first}/, '')
# Remove leading "/" to avoid generating urls with `//`
result.gsub(%r!/:#{token.first}!, "")

This comment has been minimized.

@envygeeks

envygeeks May 28, 2016

Contributor

Why isn't this escaped? /cc @jekyll/core.

@envygeeks

envygeeks May 28, 2016

Contributor

Why isn't this escaped? /cc @jekyll/core.

This comment has been minimized.

@parkr

parkr Jun 1, 2016

Member

@envygeeks Why isn't token.first escaped? Not sure.

@parkr

parkr Jun 1, 2016

Member

@envygeeks Why isn't token.first escaped? Not sure.

template.gsub(/:([a-z_]+)/.freeze) do |match|
replacement = @placeholders.public_send(match.sub(':'.freeze, ''.freeze))
template.gsub(/:([a-z_]+)/) do |match|
replacement = @placeholders.public_send(match.sub(":".freeze, "".freeze))

This comment has been minimized.

@envygeeks

envygeeks May 28, 2016

Contributor

If we are gonna do "".freeze everywhere, we need do like Liquid does and create EMPTY_STRING constant, because this is nasty and overwhelming. /cc @jekyll/core

@envygeeks

envygeeks May 28, 2016

Contributor

If we are gonna do "".freeze everywhere, we need do like Liquid does and create EMPTY_STRING constant, because this is nasty and overwhelming. /cc @jekyll/core

Show outdated Hide outdated lib/jekyll/url.rb
end
# Returns a sanitized String URL, stripping "../../" and multiples of "/",
# as well as the beginning "/" so we can enforce and ensure it.
def sanitize_url(str)
"/" + str.gsub(/\/{2,}/, "/").gsub(/\.+\/|\A\/+/, "")
"/" + str.gsub(%r!/{2,}!, "/").gsub(%r!\.+\/|\A\/+!, "")

This comment has been minimized.

@envygeeks

envygeeks May 28, 2016

Contributor

%r!\.+/|\A/+!

@envygeeks

envygeeks May 28, 2016

Contributor

%r!\.+/|\A/+!

This comment has been minimized.

@derekgottlieb

derekgottlieb May 28, 2016

Contributor

Good catch. Would it be worth adding a test for this method since nothing caught this?

@derekgottlieb

derekgottlieb May 28, 2016

Contributor

Good catch. Would it be worth adding a test for this method since nothing caught this?

This comment has been minimized.

@parkr

parkr Jun 3, 2016

Member

@derekgottlieb A test would be wonderful, yes!

@parkr

parkr Jun 3, 2016

Member

@derekgottlieb A test would be wonderful, yes!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 28, 2016

Contributor

Thanks! Just a few comments, some for you and the others for @jekyll/core.

Contributor

envygeeks commented May 28, 2016

Thanks! Just a few comments, some for you and the others for @jekyll/core.

@envygeeks envygeeks added the shipit label Jun 2, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 2, 2016

Contributor

:shipit:

Contributor

envygeeks commented Jun 2, 2016

:shipit:

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 3, 2016

Member

Thanks, Derek!

@jekyllbot: merge +dev

Member

parkr commented Jun 3, 2016

Thanks, Derek!

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 986eda3 into jekyll:master Jun 3, 2016

1 check passed

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

jekyllbot added a commit that referenced this pull request Jun 3, 2016

parkr added a commit that referenced this pull request Jun 4, 2016

Merge branch 'master' into percent_r
* master: (22 commits)
  Update history to reflect merge of #4980 [ci skip]
  werdz
  Use jekyll-mentions and restructure
  Add post about GSoC project.
  Update history to reflect merge of #4976 [ci skip]
  Amend WEBrick default headers documentation
  Update history to reflect merge of #4966 [ci skip]
  .rubocop.yml - remove lib/jekyll.rb
  lib/jekyll.rb - fix offenses reported by rubocop method set_timezone is ignored using rubocop:disable Style/AccessorMethodName
  Update history to reflect merge of #4977 [ci skip]
  Update history to reflect merge of #4962 [ci skip]
  Update history to reflect merge of #4959 [ci skip]
  Fixed typo
  Changed github-gem to github-pages
  Included installation instructions
  Installation instructions for github-pages gem
  Added link to windows doc page
  Fix inaccurate HTTP response header field name
  Minor tweak to fix missing apostrophne
  Feedback for flubbed regex and prefer File's directory check
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment