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

Add --show-linter-names option #340

Merged
merged 1 commit into from
May 24, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jan 13, 2024

Closes #336

This PR adds a --show-linter-names option. It prepends each offence with the linter name.

It applies only to the Multiline and Compact reporters. The JSON and JUnit reporters already include the linter name, so I have left them as-is.

Note: If RuboCop is enabled, RuboCop offences will display as:

[Rubocop] Layout/TrailingEmptyLines: Final newline missing.
In file: app/views/announcements/index.html.erb:2

(the cop name part was already there, but having the [RuboCop] prefix now makes it easier to distinguish the source of each offence).

@andyw8 andyw8 force-pushed the andyw8/add-linter-names-option branch from 58019ac to 804c838 Compare January 13, 2024 21:30
@andyw8 andyw8 changed the title (WIP) Add linter names option Add --show-linter-names option Jan 13, 2024
@andyw8 andyw8 marked this pull request as ready for review January 13, 2024 21:48
@andyw8 andyw8 force-pushed the andyw8/add-linter-names-option branch 2 times, most recently from 8062d65 to ec580e6 Compare January 13, 2024 21:50
@andyw8
Copy link
Contributor Author

andyw8 commented Jan 13, 2024

As soon as I opened this, I spotted that @Earlopain already has a similar PR: #328

The main differences are:

  • This one applies to the Compact reporter as well as Multiline
  • This one needs users to pass the command-line option rather than enabling it by default.
  • This one display the linter name in square brackets (not important).

@Earlopain
Copy link
Contributor

Your implementation is better, I've closed my PR. I do think it should be enabled by default though, same as RuboCop does it.

They print cop names if there is more than one cop enabled, so if you run with --only Lint/Abc the cop name will be omitted since it is unambiguous. Seems to be --enable-linters here, however I don't know how that interacts with the rubocop cops.

@andyw8
Copy link
Contributor Author

andyw8 commented Jan 14, 2024

Thanks @Earlopain

My only concern about enabling it by default is that someone may have built tooling based on the current report format (although the JSON reporter would have been a better choice).

I'll wait for others to weigh in on whether to make this the default behaviour.

@andyw8
Copy link
Contributor Author

andyw8 commented Jan 15, 2024

Note: If allowing this to be set, we should probably also add a ShowLinterNames option for the config file.

@@ -74,7 +74,7 @@ def run(args = ARGV)
@stats.linters = enabled_linter_classes.size
@stats.autocorrectable_linters = enabled_linter_classes.count(&:support_autocorrect?)

reporter = Reporter.create_reporter(@options[:format], @stats, autocorrect?)
reporter = Reporter.create_reporter(@options[:format], @stats, autocorrect?, @options[:show_linter_names])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(if we need to add more options in future, it might be better to change this to pass the options hash, rather than individual options).

@andyw8 andyw8 force-pushed the andyw8/add-linter-names-option branch from ec580e6 to ba09d0b Compare May 23, 2024 17:49
@andyw8
Copy link
Contributor Author

andyw8 commented May 23, 2024

Rebased to resolve conflicts.

@andyw8 andyw8 merged commit f82b3f7 into Shopify:main May 24, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show linter names in CLI output?
3 participants