Rubocop: features #4934

Merged
merged 2 commits into from May 26, 2016

Conversation

Projects
None yet
5 participants
@pathawks
Member

pathawks commented May 24, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks May 24, 2016

Member
features/support/formatter.rb:110:29: C: Avoid parameter lists longer than 4 parameters.
      def before_step_result(_keyword, _step_match, _multiline_arg, status, exception, \ ...

features/support/formatter.rb:133:20: C: Avoid parameter lists longer than 4 parameters.
      def step_name(_keyword, _step_match, status, _source_indent, _background, _file_colon_line)

Is there any reason for the unused parameters? I could pull them out, but I just want to make sure they're not there to maintain consistency with an other API or something.

features/support/helpers.rb:99:1: C: Assignment Branch Condition size for run_in_shell is too high. [22.14/20]
def run_in_shell(*args)

I am reluctant to modify this function because it seems pretty concise. I am not sure if anything I could modify would make any improvement 😕

Member

pathawks commented May 24, 2016

features/support/formatter.rb:110:29: C: Avoid parameter lists longer than 4 parameters.
      def before_step_result(_keyword, _step_match, _multiline_arg, status, exception, \ ...

features/support/formatter.rb:133:20: C: Avoid parameter lists longer than 4 parameters.
      def step_name(_keyword, _step_match, status, _source_indent, _background, _file_colon_line)

Is there any reason for the unused parameters? I could pull them out, but I just want to make sure they're not there to maintain consistency with an other API or something.

features/support/helpers.rb:99:1: C: Assignment Branch Condition size for run_in_shell is too high. [22.14/20]
def run_in_shell(*args)

I am reluctant to modify this function because it seems pretty concise. I am not sure if anything I could modify would make any improvement 😕

features/support/formatter.rb
@@ -107,6 +107,7 @@ def before_step(step)
#
+ # rubocop:disable Metrics/ParameterLists

This comment has been minimized.

@pathawks

pathawks May 24, 2016

Member

⚠️ I am cheating

@pathawks

pathawks May 24, 2016

Member

⚠️ I am cheating

This comment has been minimized.

@envygeeks

envygeeks May 24, 2016

Contributor

💯 Increase the size in Rubocop. 6 is a good number if you ask me, if it's over size I don't know what we should increase it to. I vote increase it mostly because Rubocop does count kwd's, so since it's already impacting us without them, it'll impact us more with them if we ever do use them (which we do in a few places.)

@envygeeks

envygeeks May 24, 2016

Contributor

💯 Increase the size in Rubocop. 6 is a good number if you ask me, if it's over size I don't know what we should increase it to. I vote increase it mostly because Rubocop does count kwd's, so since it's already impacting us without them, it'll impact us more with them if we ever do use them (which we do in a few places.)

@@ -96,6 +96,7 @@ def run_jekyll(args)
#
+# rubocop:disable Metrics/AbcSize

This comment has been minimized.

@pathawks

pathawks May 24, 2016

Member

⚠️ I am cheating

@pathawks

pathawks May 24, 2016

Member

⚠️ I am cheating

#
def slug(title = nil)
if !title
then Time.now.strftime("%s%9N") # nanoseconds since the Epoch
- else title.downcase.gsub(/[^\w]/, " ").strip.gsub(/\s+/, '-')
+ else title.downcase.gsub(/[^\w]/, " ").strip.gsub(/\s+/, "-")
end
end
#

This comment has been minimized.

@pathawks

pathawks May 24, 2016

Member

@envygeeks What is the purpose of the # separating some methods?
Seems to have been added in #4343 #4347

@pathawks

pathawks May 24, 2016

Member

@envygeeks What is the purpose of the # separating some methods?
Seems to have been added in #4343 #4347

This comment has been minimized.

@envygeeks

envygeeks May 24, 2016

Contributor

I audit code so much in my daily life that normally add comments between methods and where there are no comments I either leave "# --" or "#" because it helps to spot a coming method quickly (lets admit it, we all have trouble finding a def at some point when looking through a large file or object.)

@parkr doesn't like it so when he mentions it I remove them (as my editor -- Atom -- has a plugin I built to automatically do that.) Some may argue that's why we have symbol finders, but I argue that when auditing code, I'm hardly interested in that and at a normal basis I am too because I feel that a file that cannot be browsed without a symbol browser and without syntax highlighting is no code that should be unleashed upon the world.

Long explanation, sorry.

@envygeeks

envygeeks May 24, 2016

Contributor

I audit code so much in my daily life that normally add comments between methods and where there are no comments I either leave "# --" or "#" because it helps to spot a coming method quickly (lets admit it, we all have trouble finding a def at some point when looking through a large file or object.)

@parkr doesn't like it so when he mentions it I remove them (as my editor -- Atom -- has a plugin I built to automatically do that.) Some may argue that's why we have symbol finders, but I argue that when auditing code, I'm hardly interested in that and at a normal basis I am too because I feel that a file that cannot be browsed without a symbol browser and without syntax highlighting is no code that should be unleashed upon the world.

Long explanation, sorry.

@@ -104,8 +107,9 @@ def before_step(step)
#
- def before_step_result(keyword, step_match, multiline_arg, status, exception, \
- source_indent, background, file_colon_line)
+ # rubocop:disable Metrics/ParameterLists

This comment has been minimized.

@pathawks

pathawks May 25, 2016

Member

⚠️ I am cheating

@pathawks

pathawks May 25, 2016

Member

⚠️ I am cheating

features/support/formatter.rb
- source_indent, background, file_colon_line)
+ # rubocop:disable Metrics/ParameterLists
+ def before_step_result(_keyword, _step_match, _multiline_arg, status, exception, \
+ _source_indent, background, _file_colon_line)

This comment has been minimized.

@envygeeks

envygeeks May 25, 2016

Contributor

Indent to the first word and then indent twice past that:

def before_step_result(args1,
        arg2)

  # Stuff
end
@envygeeks

envygeeks May 25, 2016

Contributor

Indent to the first word and then indent twice past that:

def before_step_result(args1,
        arg2)

  # Stuff
end

This comment has been minimized.

@pathawks

pathawks May 26, 2016

Member

@@ -34,7 +34,7 @@ def initialize(runtime, path_or_io, options)
#
- def before_features(features)
+ def before_features(_features)

This comment has been minimized.

@envygeeks

envygeeks May 25, 2016

Contributor

Since this is an implementation on a reference I vote just do (_) instead of the full name. People can always look at the reference class for the implementation that they need and what arguments it accepts, they just need know we don't care about them here.

@envygeeks

envygeeks May 25, 2016

Contributor

Since this is an implementation on a reference I vote just do (_) instead of the full name. People can always look at the reference class for the implementation that they need and what arguments it accepts, they just need know we don't care about them here.

This comment has been minimized.

@envygeeks

envygeeks May 25, 2016

Contributor

I take that back, it looks like we aren't referencing their implementation as a sub-class. Carry on.

@envygeeks

envygeeks May 25, 2016

Contributor

I take that back, it looks like we aren't referencing their implementation as a sub-class. Carry on.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 25, 2016

Contributor

Thanks! Just a few comments.

Contributor

envygeeks commented May 25, 2016

Thanks! Just a few comments.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks May 26, 2016

Member

Should be good to go 👍

Member

pathawks commented May 26, 2016

Should be good to go 👍

end
#
def seconds_agnostic_time(time)
time = time.strftime("%H:%M:%S") if time.is_a?(Time)
- hour, minutes, _ = time.split(":")
+ hour, minutes, = time.split(":")

This comment has been minimized.

@parkr

parkr May 26, 2016

Member

huuuuh? this seems like a syntax error?

@parkr

parkr May 26, 2016

Member

huuuuh? this seems like a syntax error?

This comment has been minimized.

@pathawks

pathawks May 26, 2016

Member

The other way seemed clearer to me too, but Rubocop wants what Rubocop wants.

Maybe when things settle down, I will dig into this a bit more to figure out why RC wanted this changes.

@pathawks

pathawks May 26, 2016

Member

The other way seemed clearer to me too, but Rubocop wants what Rubocop wants.

Maybe when things settle down, I will dig into this a bit more to figure out why RC wanted this changes.

This comment has been minimized.

@pathawks

pathawks May 27, 2016

Member

It is a violation of Style/TrailingUnderscoreVariable

I guess a "better" way to do this would be

hour, minutes = time.split(":")
@pathawks

pathawks May 27, 2016

Member

It is a violation of Style/TrailingUnderscoreVariable

I guess a "better" way to do this would be

hour, minutes = time.split(":")

This comment has been minimized.

@TWiStErRob

TWiStErRob Sep 21, 2016

Contributor

I don't know about RC, but some tools allow named exceptions to a rule, something like:

hour, minutes, ignore = time.split(":")
hour, minutes, ignore_seconds = time.split(":")

Also http://stackoverflow.com/a/33006942/253468

@TWiStErRob

TWiStErRob Sep 21, 2016

Contributor

I don't know about RC, but some tools allow named exceptions to a rule, something like:

hour, minutes, ignore = time.split(":")
hour, minutes, ignore_seconds = time.split(":")

Also http://stackoverflow.com/a/33006942/253468

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 26, 2016

Member

Hm, it all works. Thanks again, @pathawks!!!

@jekyllbot: merge +dev

Member

parkr commented May 26, 2016

Hm, it all works. Thanks again, @pathawks!!!

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit f73b2d0 into jekyll:master May 26, 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 26, 2016

@pathawks pathawks deleted the pathawks:features branch May 26, 2016

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