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

Inherit Jekyll's rubocop config for consistency #38

Merged
merged 4 commits into from
Aug 11, 2016
Merged

Inherit Jekyll's rubocop config for consistency #38

merged 4 commits into from
Aug 11, 2016

Conversation

DirtyF
Copy link
Member

@DirtyF DirtyF commented Aug 11, 2016

Based on jekyll/jekyll-seo-tag#109

This PR is the result of adding inherit_gem:\n jekyll: .rubocop.ymlto.rubocop.yml and running rubocop -a so that this project follows Jekyll core's coding styles for consistency like other core plugins. This way, as Jekyll's Ruby style changes, so too will this project's.

Note, that this PR does not fix the last offense:

lib/jekyll-mentions.rb:12:7: C: Assignment Branch Condition size for mentionify is too high. [21.93/20] def mentionify(doc)

This PR is the result of adding inherit_gem:\n jekyll: .rubocop.yml to .rubocop.yml and running rubocop -a so that this project follows Jekyll core's coding styles for consistency like other core plugins. This way, as Jekyll's Ruby style changes, so too will this project's.

This PR does not fix the last offense:

`lib/jekyll-mentions.rb:12:7: C: Assignment Branch Condition size for mentionify is too high. [21.93/20]
      def mentionify(doc)`
end

def mention_username_pattern
Regexp.new(HTML::Pipeline::MentionFilter::UsernamePattern.source, Regexp::IGNORECASE)
Regexp.new(HTML::Pipeline::MentionFilter::UsernamePattern.source,
Regexp::IGNORECASE)
Copy link
Member

Choose a reason for hiding this comment

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

Let's separate these arguments so they each have their own line:

Regexp.new(
  Arg1,
  Arg2
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@parkr
Copy link
Member

parkr commented Aug 11, 2016

Well done! You can add a # rubocop:ignore Metrics/XXX above that method for now if you like (not sure what the class name is to ignore based on the message you had above)

@@ -15,7 +14,7 @@ def mentionify(doc)
src = mention_base(doc.site.config)
if doc.output.include? BODY_START_TAG
parsed_doc = Nokogiri::HTML::Document.parse(doc.output)
body = parsed_doc.at_css('body')
body = parsed_doc.at_css("body")
body.children = filter_with_mention(src).call(body.inner_html)[:output].to_s
doc.output = parsed_doc.to_html
else
Copy link
Contributor

Choose a reason for hiding this comment

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

To get Rubocop to not yell at us about the method, you could move the contents of the first half of the condition to a private parse_as_html or whatever method (or you can ignore it for now).

@benbalter
Copy link
Contributor

Nice work!

@DirtyF
Copy link
Member Author

DirtyF commented Aug 11, 2016

@parkr I applied your recommendations

@benbalter Sorry, I am not sure how to proper do this 😨

@benbalter
Copy link
Contributor

Nice. I believe adding s.add_dependency "activesupport", '~> 4.0' to the Gemspec should resolve the failing tests for Ruby < 2.3

@DirtyF
Copy link
Member Author

DirtyF commented Aug 11, 2016

@benbalter tests are OK now 🎉

@benbalter
Copy link
Contributor

LGTM.

@pathawks
Copy link
Member

LGTM

@benbalter
Copy link
Contributor

Thanks @DirtyF!

@benbalter
Copy link
Contributor

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 7ebe466 into jekyll:master Aug 11, 2016
jekyllbot added a commit that referenced this pull request Aug 11, 2016
@DirtyF DirtyF deleted the jekyll-rubocop branch August 11, 2016 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants