Update `jekyll/commands*` to pass rubocop rules #4888

Merged
merged 6 commits into from May 16, 2016

Conversation

Projects
None yet
4 participants
@TheLucasMoore
Contributor

TheLucasMoore commented May 12, 2016

One done. Working on the rest of commands folder now.

lib/jekyll/commands/clean.rb
- c.description 'Clean the site (removes site output and metadata file) without building.'
+ c.syntax "clean [subcommand]"
+ c.description "Clean the site
+ (removes site output and metadata file) without building."

This comment has been minimized.

@envygeeks

envygeeks May 12, 2016

Contributor

This might output literally and it's hard for static analysis to catch this. Try:

"string1 " \
  "string2"
@envygeeks

envygeeks May 12, 2016

Contributor

This might output literally and it's hard for static analysis to catch this. Try:

"string1 " \
  "string2"

This comment has been minimized.

@TheLucasMoore

TheLucasMoore May 12, 2016

Contributor

Yes, I noticed that in later files. Fixed in this coming PR. Thanks @envygeeks.

@TheLucasMoore

TheLucasMoore May 12, 2016

Contributor

Yes, I noticed that in later files. Fixed in this coming PR. Thanks @envygeeks.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 12, 2016

Contributor

Thanks for your contribution! Just a comment, and thanks again!

Contributor

envygeeks commented May 12, 2016

Thanks for your contribution! Just a comment, and thanks again!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 13, 2016

Contributor

Accidental close?

Contributor

envygeeks commented May 13, 2016

Accidental close?

@TheLucasMoore TheLucasMoore reopened this May 13, 2016

@TheLucasMoore

This comment has been minimized.

Show comment
Hide comment
@TheLucasMoore

TheLucasMoore May 13, 2016

Contributor

Yeah, sorry. I see now that commits continually update here. Currently running tests.

Contributor

TheLucasMoore commented May 13, 2016

Yeah, sorry. I see now that commits continually update here. Currently running tests.

@TheLucasMoore TheLucasMoore changed the title from clean.rb passing rubocop to jekyll/commands passing rubocop May 13, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 13, 2016

Member

@TheLucasMoore 😍 Thank you so much for the pull request!

One easier way to see if the changes will work is to run script/fmt from your computer locally. It should work fine on Windows, because Rubocop is (I think?) pure Ruby. You should be able to see the failures there and can commit and push once they're all fixed. Two left!

lib/jekyll/commands/new.rb:21:9: C: Metrics/AbcSize: Assignment Branch Condition size for process is too high. [23.22/20]
        def process(args, options = {})
        ^^^
lib/jekyll/commands/serve.rb:174:9: C: Metrics/AbcSize: Assignment Branch Condition size for enable_ssl is too high. [29.7/20]
        def enable_ssl(opts)
        ^^^
Member

parkr commented May 13, 2016

@TheLucasMoore 😍 Thank you so much for the pull request!

One easier way to see if the changes will work is to run script/fmt from your computer locally. It should work fine on Windows, because Rubocop is (I think?) pure Ruby. You should be able to see the failures there and can commit and push once they're all fixed. Two left!

lib/jekyll/commands/new.rb:21:9: C: Metrics/AbcSize: Assignment Branch Condition size for process is too high. [23.22/20]
        def process(args, options = {})
        ^^^
lib/jekyll/commands/serve.rb:174:9: C: Metrics/AbcSize: Assignment Branch Condition size for enable_ssl is too high. [29.7/20]
        def enable_ssl(opts)
        ^^^
lib/jekyll/commands/serve.rb
@@ -174,19 +174,22 @@ def enable_logging(opts)
def enable_ssl(opts)
return if !opts[:JekyllOptions]["ssl_cert"] && !opts[:JekyllOptions]["ssl_key"]
if !opts[:JekyllOptions]["ssl_cert"] || !opts[:JekyllOptions]["ssl_key"]
- raise RuntimeError, "--ssl-cert or --ssl-key missing."
+ raise RuntimeError

This comment has been minimized.

@parkr

parkr May 13, 2016

Member

Can you add this message back? Otherwise it's confusing. 😄

@parkr

parkr May 13, 2016

Member

Can you add this message back? Otherwise it's confusing. 😄

lib/jekyll/commands/serve/servlet.rb
@@ -27,7 +27,7 @@ def search_file(req, res, basename)
#
- def do_GET(req, res)
+ def do_get(req, res)

This comment has been minimized.

@parkr

parkr May 13, 2016

Member

I believe this is a special method so you can add a comment above the method to disable whatever it violates via # rubocop:disable <copname> e.g. # rubocop:disable Metrics/LineLength.

@parkr

parkr May 13, 2016

Member

I believe this is a special method so you can add a comment above the method to disable whatever it violates via # rubocop:disable <copname> e.g. # rubocop:disable Metrics/LineLength.

This comment has been minimized.

@envygeeks

envygeeks May 13, 2016

Contributor

It is, this is an upstream method that we override upon subclassing. So in order for us to fix this upstream would have to fix it.

@envygeeks

envygeeks May 13, 2016

Contributor

It is, this is an upstream method that we override upon subclassing. So in order for us to fix this upstream would have to fix it.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 13, 2016

Member

@TheLucasMoore The ABC Size cop is about complexity of the method. The way to fix it is by breaking big methods into smaller methods that do the if/else's and so on.

Member

parkr commented May 13, 2016

@TheLucasMoore The ABC Size cop is about complexity of the method. The way to fix it is by breaking big methods into smaller methods that do the if/else's and so on.

@parkr parkr referenced this pull request May 13, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
@TheLucasMoore

This comment has been minimized.

Show comment
Hide comment
@TheLucasMoore

TheLucasMoore May 13, 2016

Contributor

Alright, the commands subfolder should all be passing now. I added rubocop exceptions for the redundant error and fixed one of the ABC metric size errors. The enable_ssl private method in serve.rb is passing with a rubocop exception. I tried a few ways of breaking it apart, but it seems like an exception is the most efficient way to have it passing.

Contributor

TheLucasMoore commented May 13, 2016

Alright, the commands subfolder should all be passing now. I added rubocop exceptions for the redundant error and fixed one of the ABC metric size errors. The enable_ssl private method in serve.rb is passing with a rubocop exception. I tried a few ways of breaking it apart, but it seems like an exception is the most efficient way to have it passing.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 13, 2016

Contributor

I'm gonna be honest, looking at all these lines that have to shift down decreasing readability makes me think two things (which are exclusive:)

  1. We need to rewrite those files to be cleaner to begin with, they shift a lot, this is hindering readability extraordinarily (not @TheLucasMoore's fault, it's our fault.)
  2. We need to increase the length we allow for lines (I'm almost against this because sometimes I work on tiny screens and it would make it hard to audit a bug in that case.)

Ultimately though, reviewing, most of it, it LGTM.

Contributor

envygeeks commented May 13, 2016

I'm gonna be honest, looking at all these lines that have to shift down decreasing readability makes me think two things (which are exclusive:)

  1. We need to rewrite those files to be cleaner to begin with, they shift a lot, this is hindering readability extraordinarily (not @TheLucasMoore's fault, it's our fault.)
  2. We need to increase the length we allow for lines (I'm almost against this because sometimes I work on tiny screens and it would make it hard to audit a bug in that case.)

Ultimately though, reviewing, most of it, it LGTM.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 13, 2016

Contributor

/cc @jekyll/core

Contributor

envygeeks commented May 13, 2016

/cc @jekyll/core

lib/jekyll/commands/new.rb
- f.write(gemfile_contents)
- end
- end
+ if_options_blank(new_blog_path, options)

This comment has been minimized.

@parkr

parkr May 13, 2016

Member

Why don't we have the if still in this method and move just the larger part of the else into its own method:

def process(args, options = {})
  # ...

  if options["blank"]
    create_blank_site new_blog_path
  else
    create_prefilled_site new_blog_path
  end

  # ...
end

def create_prefilled_site(new_blog_path)
  create_sample_files new_blog_path

  File.open(...) do |f|
    # ...
  end

  File.open(...) do |f|
    # ..
  end
end

Does that sound good to you? Then the method doesn't start with an if.

@parkr

parkr May 13, 2016

Member

Why don't we have the if still in this method and move just the larger part of the else into its own method:

def process(args, options = {})
  # ...

  if options["blank"]
    create_blank_site new_blog_path
  else
    create_prefilled_site new_blog_path
  end

  # ...
end

def create_prefilled_site(new_blog_path)
  create_sample_files new_blog_path

  File.open(...) do |f|
    # ...
  end

  File.open(...) do |f|
    # ..
  end
end

Does that sound good to you? Then the method doesn't start with an if.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 13, 2016

Member

We need to increase the length we allow for lines (I'm almost against this

I'm ok bumping it 100 but 90 is working fine for me. The problem is really that we're so far indented that you only get 50 characters to work with.

Member

parkr commented May 13, 2016

We need to increase the length we allow for lines (I'm almost against this

I'm ok bumping it 100 but 90 is working fine for me. The problem is really that we're so far indented that you only get 50 characters to work with.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 13, 2016

Contributor

I'm ok bumping it 100 but 90 is working fine for me. The problem is really that we're so far indented that you only get 50 characters to work with.

What do you think about maybe shifting to class Jekyll::Blah::Blah for def of classes which would give us way more room. Since we autoload a lot and people normally load "jekyll" before anything I think that would be a good idea to give us more room since we have deep nesting in some places.

Contributor

envygeeks commented May 13, 2016

I'm ok bumping it 100 but 90 is working fine for me. The problem is really that we're so far indented that you only get 50 characters to work with.

What do you think about maybe shifting to class Jekyll::Blah::Blah for def of classes which would give us way more room. Since we autoload a lot and people normally load "jekyll" before anything I think that would be a good idea to give us more room since we have deep nesting in some places.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 13, 2016

Member

What do you think about maybe shifting to class Jekyll::Blah::Blah for def of classes which would give us way more room

Yeah, that's a good idea. I know that has slightly different semantics, though. Should we do

module Jekyll::Commands
  class Build < Command
  end
end

i.e. just collapse to two levels of nesting?

Member

parkr commented May 13, 2016

What do you think about maybe shifting to class Jekyll::Blah::Blah for def of classes which would give us way more room

Yeah, that's a good idea. I know that has slightly different semantics, though. Should we do

module Jekyll::Commands
  class Build < Command
  end
end

i.e. just collapse to two levels of nesting?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 13, 2016

Contributor

i.e. just collapse to two levels of nesting?

👍

Contributor

envygeeks commented May 13, 2016

i.e. just collapse to two levels of nesting?

👍

@TheLucasMoore

This comment has been minimized.

Show comment
Hide comment
@TheLucasMoore

TheLucasMoore May 14, 2016

Contributor

@envygeeks - Alright, so that change is quite a large scope which has a lot of structural implications. How should we proceed within the scope of this PR? I'm happy to continue to bring other folders into compliance with Rubocop.

Contributor

TheLucasMoore commented May 14, 2016

@envygeeks - Alright, so that change is quite a large scope which has a lot of structural implications. How should we proceed within the scope of this PR? I'm happy to continue to bring other folders into compliance with Rubocop.

@parkr parkr changed the title from jekyll/commands passing rubocop to Update `jekyll/commands*` to pass rubocop rules May 16, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 16, 2016

Member

Thanks very much for this, @TheLucasMoore!

@jekyllbot: merge +dev

Member

parkr commented May 16, 2016

Thanks very much for this, @TheLucasMoore!

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit fb860fc into jekyll:master May 16, 2016

1 check passed

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

jekyllbot added a commit that referenced this pull request May 16, 2016

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