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

[Proposal] Display cop names by default #5037

Merged
merged 1 commit into from Nov 18, 2017

Conversation

Projects
None yet
4 participants
@pocke
Copy link
Member

pocke commented Nov 11, 2017

Problem

I believe --display-cop-names is very useful option, but it is disabled by default.
Cop names are very important in using RuboCop. For example, searching with cop names, bug reporting to rubocop with cop names.
So I think RuboCop should display cop names by default.

Many linters have same behaviour, for instance:

  • Reek
    test.rb -- 3 warnings:
      [2, 3]:DuplicateMethodCall: x calls 'foo.foo' 2 times [https://github.com/troessner/reek/blob/master/docs/Duplicate-Method-Call.md]
      [1]:UncommunicativeMethodName: x has the name 'x' [https://github.com/troessner/reek/blob/master/docs/Uncommunicative-Method-Name.md]
      [1]:UtilityFunction: x doesn't depend on instance state (maybe move it to another class?) [https://github.com/troessner/reek/blob/master/docs/Utility-Function.md]
    
  • ESLint
    /tmp/tmp.GhjIgnTwAu/test.js
      1:5  error  'a' is defined but never used  no-unused-vars
      1:9  error  Missing semicolon              semi
    
    ✖ 2 problems (2 errors, 0 warnings)
    
  • stylelint

Solution

I propose to make --display-cop-names option to enable by default.

  • Make --display-cop-names to enable by default.
  • Add --no-display-cop-names option to disable displaying cop names.

  • Add an entry to the changelog
  • Fix the failing tests
  • Fix the rubocop's offences
    • maybe many "Line is too long" exist.
  • Update the manual.

What do you think?


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests(rake spec) are passing.
  • The new code doesn't generate RuboCop offenses that are checked by rake internal_investigation.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

@pocke pocke changed the title [WIP] Display cop names by default [Proposal][WIP] Display cop names by default Nov 11, 2017

@Drenmi

This comment has been minimized.

Copy link
Collaborator

Drenmi commented Nov 11, 2017

I tend to agree with this. 🙂 Another, related issue which has bothered me is that re-running with -D seems to clear the cached result, which is completely useless. 😅

@rrosenblum

This comment has been minimized.

Copy link
Contributor

rrosenblum commented Nov 15, 2017

@Drenmi I don't think I have ever run into an issue with -D clearing the cache before.

rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ time b rubocop -D lib/rubocop/cop/rails --cache false
Inspecting 36 files
....................................

36 files inspected, no offenses detected
bundle exec rubocop -D lib/rubocop/cop/rails --cache false  1.77s user 0.10s system 98% cpu 1.890 total
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ time b rubocop -D lib/rubocop/cop/rails              
Inspecting 36 files
....................................

36 files inspected, no offenses detected
bundle exec rubocop -D lib/rubocop/cop/rails  0.76s user 0.17s system 97% cpu 0.957 total
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ time b rubocop -D lib/rubocop/cop/rails
Inspecting 36 files
....................................

36 files inspected, no offenses detected
bundle exec rubocop -D lib/rubocop/cop/rails  0.74s user 0.17s system 97% cpu 0.934 total

rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ b rubocop -V
0.51.0 (using Parser 2.4.0.0, running on ruby 2.4.2 x86_64-darwin16)

It worked fine for me running against master. Did you have other options enabled when ran into the issue?

@Drenmi

This comment has been minimized.

Copy link
Collaborator

Drenmi commented Nov 15, 2017

@rrosenblum: What I meant was when you run without -D first, then run again using -D. Nothing changed, so it would be reasonable to believe it could just use the cache and display the cop names. 🙂

@pocke

This comment has been minimized.

Copy link
Member Author

pocke commented Nov 16, 2017

I tend to agree with this. 🙂 Another, related issue which has bothered me is that re-running with -D seems to clear the cached result, which is completely useless. 😅

We can add display_cop_names to here.
https://github.com/bbatsov/rubocop/blob/3122cc78df8716c9fdfc93ee7098e0ae69a11866/lib/rubocop/result_cache.rb#L10-L11

But unfortunately it does not work.
add_offense method create a new offense with an annotated offense message.
https://github.com/bbatsov/rubocop/blob/3122cc78df8716c9fdfc93ee7098e0ae69a11866/lib/rubocop/cop/cop.rb#L194-L198
So rubocop caches "annotated" messages.
To cache with -D, rubocop should annotate after caching.

For example:

# cop.rb
def add_offense
  Offense.new(message: not_annotated_message)
end

# offense.rb
def message
  MessageAnnotator.annotate(@message)
end

@pocke pocke force-pushed the pocke:DisplayCopNames branch 3 times, most recently from 1848cac to 9c24a03 Nov 16, 2017

@pocke pocke force-pushed the pocke:DisplayCopNames branch from 9c24a03 to 3a020b4 Nov 17, 2017

@pocke

This comment has been minimized.

Copy link
Member Author

pocke commented Nov 17, 2017

It's all done!
@bbatsov Can you review this pull-request?

@pocke pocke changed the title [Proposal][WIP] Display cop names by default [Proposal] Display cop names by default Nov 17, 2017

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Nov 17, 2017

Looks good overall. But if we're making this mainstream I want us to make one more change - color cops (possibly different colors for different cop groups) for the cop names stand out more in the messages.

@pocke

This comment has been minimized.

Copy link
Member Author

pocke commented Nov 18, 2017

Thank you.

But if we're making this mainstream I want us to make one more change - color cops (possibly different colors for different cop groups) for the cop names stand out more in the messages.

Sorry, I'm not sure what did you mean.
I guess cop names, right?

before
171117204539

after
171117204531

It is reasonable to me. But I think we should implement it on another pull-request.

@bbatsov bbatsov merged commit 23876e2 into rubocop-hq:master Nov 18, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Nov 18, 2017

I guess cop names, right?

Yeah, that's what I meant.

It is reasonable to me. But I think we should implement it on another pull-request.

Fine by me.

@pocke pocke deleted the pocke:DisplayCopNames branch Nov 18, 2017

@ilovezfs ilovezfs referenced this pull request Jan 21, 2018

Merged

rubocop: don't always display cop names. #3710

3 of 5 tasks complete

pocke added a commit to pocke/rubocop that referenced this pull request Mar 15, 2018

Update the formatter manual
This pull-request updates `manual/formatters.md`.
The manual has some outdated contents, so I updates them.
It change two things.

Firstly, add cop name to the outputs. See rubocop-hq#5037

Secondly, update `--format offenses` example. Currently the formatter outputs cop full names (e.g. Metrics/LineLength), but the example does not display full name (e.g. LineLength). So I updated the example.
And I use an real output in a project as the example. Because I'm not sure the all departments of the current examples, and I don't know they still live (probably some cops has been renamed). So I replaced whole the example.

@pocke pocke referenced this pull request Mar 15, 2018

Merged

Update the formatter manual #5690

bbatsov added a commit that referenced this pull request Mar 16, 2018

Update the formatter manual
This pull-request updates `manual/formatters.md`.
The manual has some outdated contents, so I updates them.
It change two things.

Firstly, add cop name to the outputs. See #5037

Secondly, update `--format offenses` example. Currently the formatter outputs cop full names (e.g. Metrics/LineLength), but the example does not display full name (e.g. LineLength). So I updated the example.
And I use an real output in a project as the example. Because I'm not sure the all departments of the current examples, and I don't know they still live (probably some cops has been renamed). So I replaced whole the example.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.