Rubocop jekyll.rb #4966

Merged
merged 2 commits into from Jun 3, 2016

Conversation

Projects
None yet
5 participants
@praveen612
Contributor

praveen612 commented May 30, 2016

fixes one file from #4885

work in progress

yet to fix below offence reported by rubocop

lib/jekyll.rb:119:9: C: Do not prefix writer method names with set_.
    def set_timezone(timezone)
        ^^^^^^^^^^^^
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 2, 2016

Contributor

hey @floydpraveen do you mind adding # rubocop:disable Style/AccessorMethodName above set_timezone and then below it and above the next method name do # rubocop:enable Style/AccessorMethodName

This is :shipit: after that minor change!
Thanks!

Contributor

envygeeks commented Jun 2, 2016

hey @floydpraveen do you mind adding # rubocop:disable Style/AccessorMethodName above set_timezone and then below it and above the next method name do # rubocop:enable Style/AccessorMethodName

This is :shipit: after that minor change!
Thanks!

@praveen612

This comment has been minimized.

Show comment
Hide comment
@praveen612

praveen612 Jun 2, 2016

Contributor

@envygeeks done, thanks for reviewing it

Contributor

praveen612 commented Jun 2, 2016

@envygeeks done, thanks for reviewing it

@praveen612

This comment has been minimized.

Show comment
Hide comment
@praveen612

praveen612 Jun 2, 2016

Contributor

@envygeeks let me reword the commit messages

Contributor

praveen612 commented Jun 2, 2016

@envygeeks let me reword the commit messages

@envygeeks envygeeks changed the title from make lib/jekyll.rb rubocop compliant to Rubocop jekyll.rb Jun 2, 2016

@envygeeks envygeeks changed the title from Rubocop jekyll.rb to Rubocop Jekyll Jun 2, 2016

end
+ # rubocop:enable Style/AccessorMethodName

This comment has been minimized.

@parkr

parkr Jun 3, 2016

Member

I don't think this is necessary 😄 It's just applies to the method when it's right above the method like that, I'm pretty sure.

@parkr

parkr Jun 3, 2016

Member

I don't think this is necessary 😄 It's just applies to the method when it's right above the method like that, I'm pretty sure.

This comment has been minimized.

@envygeeks

envygeeks Jun 3, 2016

Contributor

@parkr it continues for the entire file, though you are right that it isn't necessary.

@envygeeks

envygeeks Jun 3, 2016

Contributor

@parkr it continues for the entire file, though you are right that it isn't necessary.

This comment has been minimized.

@praveen612

praveen612 Jun 3, 2016

Contributor

@parkr @envygeeks since it continues for the entire file, let it be there ? so new method added would be checked by rubocop

@praveen612

praveen612 Jun 3, 2016

Contributor

@parkr @envygeeks since it continues for the entire file, let it be there ? so new method added would be checked by rubocop

This comment has been minimized.

@envygeeks

envygeeks Jun 3, 2016

Contributor

@floydpraveen yessum

@envygeeks

envygeeks Jun 3, 2016

Contributor

@floydpraveen yessum

This comment has been minimized.

@pathawks

pathawks Jun 3, 2016

Member

@floydpraveen 👍

@pathawks

pathawks Jun 3, 2016

Member

@floydpraveen 👍

lib/jekyll.rb
clean_path = File.expand_path(questionable_path, "/")
- clean_path.sub!(/\A\w\:\//, '/')
+ clean_path.sub!(%r!\A\w\:\/!, "/")

This comment has been minimized.

@parkr

parkr Jun 3, 2016

Member

Correct me if I'm wrong, @envygeeks, but I think this is %r!\A\w\:/!.

@parkr

parkr Jun 3, 2016

Member

Correct me if I'm wrong, @envygeeks, but I think this is %r!\A\w\:/!.

This comment has been minimized.

@envygeeks

envygeeks Jun 3, 2016

Contributor

%r!\A\w:/!

@envygeeks

envygeeks Jun 3, 2016

Contributor

%r!\A\w:/!

lib/jekyll.rb
- if clean_path.start_with?(base_directory.sub(/\A\w\:\//, '/'))
+ if clean_path.start_with?(base_directory.sub(%r!\A\w\:\/!, "/"))

This comment has been minimized.

@parkr

parkr Jun 3, 2016

Member

Same regexp here as above, I guess? Maybe we can pull that out into a constant.

@parkr

parkr Jun 3, 2016

Member

Same regexp here as above, I guess? Maybe we can pull that out into a constant.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 3, 2016

Member

Looking pretty good! Just needs a rebase and I left a couple comments above. ❤️

Member

parkr commented Jun 3, 2016

Looking pretty good! Just needs a rebase and I left a couple comments above. ❤️

@parkr parkr added the pending-rebase label Jun 3, 2016

@parkr parkr changed the title from Rubocop Jekyll to Rubocop jekyll.rb Jun 3, 2016

@praveen612

This comment has been minimized.

Show comment
Hide comment
@praveen612

praveen612 Jun 3, 2016

Contributor

@parkr @envygeeks made the changes, thanks for the comments

Contributor

praveen612 commented Jun 3, 2016

@parkr @envygeeks made the changes, thanks for the comments

lib/jekyll.rb
- if clean_path.start_with?(base_directory.sub(/\A\w\:\//, '/'))
+

This comment has been minimized.

@envygeeks

envygeeks Jun 3, 2016

Contributor

This is causing your formatting specs to fail: you have two new lines instead of one.

@envygeeks

envygeeks Jun 3, 2016

Contributor

This is causing your formatting specs to fail: you have two new lines instead of one.

This comment has been minimized.

@praveen612

praveen612 Jun 3, 2016

Contributor

removed it

@praveen612

praveen612 Jun 3, 2016

Contributor

removed it

praveen612 added some commits Jun 2, 2016

lib/jekyll.rb - fix offenses reported by rubocop
method set_timezone is ignored using rubocop:disable Style/AccessorMethodName
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 3, 2016

Contributor

@jekyllbot: merge +dev

Contributor

envygeeks commented Jun 3, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit e2ae0ba 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

@praveen612 praveen612 deleted the praveen612:rubocop-compliant branch 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
  ...

@parkr parkr referenced this pull request Jun 15, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment