[Fix #3666] Adds Uncommunicative MethodArg & BlockParam Naming cops #5211

garettarrowood · 2017-12-11T02:34:27Z

Adds new Naming/UncommunicativeMethodArgName & Naming/UncommunicativeBlockParamName cops. This enhancement was discussed in detail on issue #3666. One detail left off the discussion was that there is now a Naming namespace, and it makes sense to add these there.

These cops are disabled by default and should be easily extendable. I'd be happy to add more configuration, but it may be best to flush out what exactly comes next in an issue.

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

bbatsov · 2017-12-11T06:40:57Z

These cops are disabled by default and should be easily extendable. I'd be happy to add more configuration, but it may be best to flush out what exactly comes next in an issue.

Out of curiosity - how many offences does the RuboCop codebase produce?

garettarrowood · 2017-12-11T12:40:28Z

1076 files inspected, 101 offenses detected

And 2 understandable cli_spec failures. However, doing this has highlighted a bug in this PR. I have to account for default args (I guess only operate on the lhs when the arg assigns). So this is WIP until I fix it. E.g.

lib/rubocop/target_finder.rb:107:29: C: Naming/UncommunicativeMethodArgName: Only use lowercase characters for method argument.
    def target_files_in_dir(base_dir = Dir.pwd)
                            ^^^^^^^^^^^^^^^^^^

The vast majority of those 101 offenses are legitimate based on standards these new cops present. Only a small handful are default argument issues. Would you like me to fix the offenses and default these to enabled?

bbatsov · 2017-12-12T11:51:21Z

The vast majority of those 101 offenses are legitimate based on standards these new cops present. Only a small handful are default argument issues. Would you like me to fix the offenses and default these to enabled?

That would be my preference, yes.

pocke · 2017-12-12T12:48:31Z

lib/rubocop/cop/mixin/uncommunicative_name.rb

+
+      def min_length
+        cop_config['MinParamNameLength'] ||
+          cop_config['MinArgNameLength']


This method is confusing. We will be able to set MinParamNameLength to Naming/UncommunicativeMethodArgName:. I think it is a bug. Naming/UncommunicativeMethodArgName should only use MinArgNameLength, do not use MinParamNameLength.

For example:

# in the mixin def min_length cop_config[MIN_LENGTH_KEY] end # in the cops class UncommunicativeMethodArgName MIN_LENGTH_KEY = 'MinArgNameLength' end

Great point! Thanks!

I wasn't able to do exactly this. Since the mixin is a Module, it didn't have access to constants from the Class it extends. But this issue is resolved.

pocke · 2017-12-12T12:51:41Z

lib/rubocop/cop/mixin/uncommunicative_name.rb

+      end
+
+      def uppercase?(name)
+        name =~ /[A-Z]/


Probably it should be [[:upper:]].
See #5017

Excellent! Thank you!

pocke · 2017-12-12T12:54:09Z

lib/rubocop/cop/naming/uncommunicative_method_arg_name.rb

+        def on_def(node)
+          return unless node.arguments?
+          check(node, node.arguments)
+        end


This cop should check defs node also (For def self.foo; end).

bbatsov · 2017-12-12T13:45:04Z

config/default.yml

@@ -685,6 +685,15 @@ Naming/PredicateName:
  Exclude:
    - 'spec/**/*'

+Naming/UncommunicativeBlockParamName:
+  # Parameter names may be equal to or greater than this value
+  MinParamNameLength: 1


Btw, with such a default will this ever flag anything? :-)

It will only flag BlockParams that end in numbers or contain capital letters. It will never flag length. But, should users want to enforce a 2 or 3 char min length, they have the ability to.

If we raise it, I think we'd have to add more config to whitelist individual letter exceptions. As we discussed some on the issue, single letter block params are more common. Perhaps we can hash it out in a new issue after this becomes deliverable?

Drenmi · 2017-12-14T15:00:52Z

lib/rubocop/cop/layout/space_around_equals_in_parameter_default.rb

@@ -70,7 +70,7 @@ def no_surrounding_space?(arg, equals)
          !arg.space_after? && !equals.space_after?
        end

-        def message(_)
+        def message(_msg)


The argument to #message is not a message. It is the offending node. 🙂

Thanks! I'll change it here and everywhere else this PR corrects a #message.

Drenmi · 2017-12-14T15:03:14Z

lib/rubocop/cop/mixin/hash_alignment.rb

@@ -10,7 +10,7 @@ def checkable_layout?(_node)
          true
        end

-        def deltas_for_first_pair(*)
+        def deltas_for_first_pair(*_args)


This isn't any more communicative than just *. 🙂

Drenmi · 2017-12-14T15:08:32Z

I think some of the fixes made here highlight the limitations of the method. The sets "short" and "uncommunicative" are not perfectly overlapping. For example, x and y are communicative for a Point class, and args (several "fixes" in the PR use this) is uncommunicative for any method 🙂

Not sure how to approach this though. Perhaps a whitelist might be a good start?

For blocks, I know there have been many discussions in the Reek repo, because people tend to use single-letter arguments for single-line blocks.

garettarrowood · 2017-12-16T01:33:45Z

I agree that these Cops have there limitations, particularly when it comes to length. To clarify:

The UncommunicativeBlockParamName sets a default length to 1. In effect, it does not enforce any length requirement. But having the config allows users to enforce a higher one should they desire.
Total agreement that arg doesn't add anything. But I'd argue it doesn't subtract anything either. And overall this did point out at least a few naming improvements. operator for op. buffer for sb. index for ix. string for s.

I'm going to give this another full pass and see if I can improve some of these names more. About half of our offenders are either t1/t2 or n1/n2. These were difficult to rename because they are often named via a each_cons(2) and thus truly seem like a first and second. I've renamed some of these token_one and token_two, and this seems horrible. I'm contemplating naming all these left_token/right_token and left_node/right_node. WDYT?

That leaves adding a whitelist. I will go ahead and add this configurable option in my next pass. Based on the corrections you see in this PR, what exactly would you like to see on rubocop's whitelist? What should be set as universal/default whitelisted names? Would t1/t2/n1/n2 be something we want to whitelist because we know they represent tokens and nodes?

Opinions & suggestions are very welcome here.

bbatsov · 2017-12-27T14:52:24Z

That leaves adding a whitelist. I will go ahead and add this configurable option in my next pass. Based on the corrections you see in this PR, what exactly would you like to see on rubocop's whitelist? What should be set as universal/default whitelisted names? Would t1/t2/n1/n2 be something we want to whitelist because we know they represent tokens and nodes?

I guess this would become apparent just by analysing the offences produced by the cop with a whitelist present. :-)

Sorry for dropping the ball on the cop for a bit, but my focus was getting 0.52.1 out.

I'm going to give this another full pass and see if I can improve some of these names more. About half of our offenders are either t1/t2 or n1/n2. These were difficult to rename because they are often named via a each_cons(2) and thus truly seem like a first and second. I've renamed some of these token_one and token_two, and this seems horrible. I'm contemplating naming all these left_token/right_token and left_node/right_node. WDYT?

token1/2 and node1/2 don't seem like bad names to me. I'm definitely open to better ideas, though. left/right convey a different meaning IMO - I immediately start thinking of lhs and rhs.

…cops

garettarrowood · 2017-12-28T21:05:08Z

lib/rubocop/cop/rails/blank.rb

-          nil_or_empty?(node) do |variable1, variable2|
-            return unless variable1 == variable2
+          nil_or_empty?(node) do |var1, var2|
+            return unless var1 == var2


I really didn't want to do this. But. For the Rails blank? and present? cops, "variable" or "var" are the best descriptions of what these values contain. (I don't find variable anymore descriptive than var.)

garettarrowood · 2017-12-28T21:08:43Z

.rubocop.yml

+    - 'token1'
+    - 'token2'
+    - 'var1'
+    - 'var2'


I find this list informative. Seems like these cops could also use a config option to turn off NamesEndingInNumbers offenses.

I was just about to suggest this. :-)

I'll add this and disable it by default as well.

bbatsov · 2017-12-29T17:09:02Z

👍

garettarrowood force-pushed the uncommunicative_names branch from 931ab1a to 918bdfe Compare December 11, 2017 03:06

pocke reviewed Dec 12, 2017

View reviewed changes

bbatsov reviewed Dec 12, 2017

View reviewed changes

garettarrowood force-pushed the uncommunicative_names branch 2 times, most recently from 70dedf0 to 4c249c3 Compare December 12, 2017 20:26

Drenmi reviewed Dec 14, 2017

View reviewed changes

garettarrowood added 3 commits December 28, 2017 13:37

[Fix rubocop#3666] Add new Uncommunicative MethodArg & BlockParam cops

3f65be3

[rubocop#3666] Add AllowedNames configuration to UncommunicativeName …

7e82e5f

…cops

[rubocop#3666] Enable Uncommunicative cops & add AllowedNames

a559956

garettarrowood force-pushed the uncommunicative_names branch from 4c249c3 to 3f11431 Compare December 28, 2017 21:02

garettarrowood commented Dec 28, 2017

View reviewed changes

[rubocop#3666] Resolve uncommunicative name offenses

76aad6c

garettarrowood force-pushed the uncommunicative_names branch from 3f11431 to 76aad6c Compare December 28, 2017 21:24

[rubocop#3666] Add AllowNamesEndingInNumbers & enable by default

cf21688

garettarrowood force-pushed the uncommunicative_names branch from dc533c5 to cf21688 Compare December 29, 2017 00:09

bbatsov merged commit 5519478 into rubocop:master Dec 29, 2017

garettarrowood deleted the uncommunicative_names branch December 29, 2017 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #3666] Adds Uncommunicative MethodArg & BlockParam Naming cops #5211

[Fix #3666] Adds Uncommunicative MethodArg & BlockParam Naming cops #5211

garettarrowood commented Dec 11, 2017 •

edited

Loading

bbatsov commented Dec 11, 2017

garettarrowood commented Dec 11, 2017 •

edited

Loading

bbatsov commented Dec 12, 2017

pocke Dec 12, 2017

garettarrowood Dec 12, 2017

garettarrowood Dec 12, 2017 •

edited

Loading

pocke Dec 12, 2017

garettarrowood Dec 12, 2017

pocke Dec 12, 2017

bbatsov Dec 12, 2017

garettarrowood Dec 12, 2017 •

edited

Loading

Drenmi Dec 14, 2017

garettarrowood Dec 14, 2017

Drenmi Dec 14, 2017

Drenmi commented Dec 14, 2017 •

edited

Loading

garettarrowood commented Dec 16, 2017

bbatsov commented Dec 27, 2017

garettarrowood Dec 28, 2017 •

edited

Loading

garettarrowood Dec 28, 2017 •

edited

Loading

bbatsov Dec 28, 2017

garettarrowood Dec 28, 2017

bbatsov commented Dec 29, 2017

[Fix #3666] Adds Uncommunicative MethodArg & BlockParam Naming cops #5211

[Fix #3666] Adds Uncommunicative MethodArg & BlockParam Naming cops #5211

Conversation

garettarrowood commented Dec 11, 2017 • edited Loading

bbatsov commented Dec 11, 2017

garettarrowood commented Dec 11, 2017 • edited Loading

bbatsov commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garettarrowood Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garettarrowood Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drenmi commented Dec 14, 2017 • edited Loading

garettarrowood commented Dec 16, 2017

bbatsov commented Dec 27, 2017

garettarrowood Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

garettarrowood Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Dec 29, 2017

garettarrowood commented Dec 11, 2017 •

edited

Loading

garettarrowood commented Dec 11, 2017 •

edited

Loading

garettarrowood Dec 12, 2017 •

edited

Loading

garettarrowood Dec 12, 2017 •

edited

Loading

Drenmi commented Dec 14, 2017 •

edited

Loading

garettarrowood Dec 28, 2017 •

edited

Loading

garettarrowood Dec 28, 2017 •

edited

Loading