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

[Fix #3542] Add IgnoreCopDirectives option to Metrics/LineLength #3583

Merged

Conversation

jmks
Copy link
Contributor

@jmks jmks commented Oct 7, 2016

Fix #3542 Since it's Hackoberfest, I thought I'd give this a shot.

This adds a IgnoreCopDirectives option to Metrics\LineLength that ignores directives like # rubocop:disable ... from counting against the line length max.

I'm unsure of a couple things:

  • Not sure if # rubocop:[dis]able type comments already have a name. In the source they seem to be called directives, so I went with that when naming the option IgnoreCopDirectives
  • I tried to run the tests/rubocop in Rubinius but all I got was seg faults. I have never tried jruby. CI has me covered?

Thanks!


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 are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

@@ -2,6 +2,8 @@

## master (unreleased)

* [#3542](https://github.com/bbatsov/rubocop/issues/3542): Add a configuration option, `IgnoreCopDirectives`, to `Metrics/LineLength` to stop cop directives (`# rubocop:disable Metrics/AbcSize`) from being counted when considering line length. ([@jmks][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be under "New Features".

@@ -1117,6 +1117,7 @@ Metrics/LineLength:
URISchemes:
- http
- https
IgnoreCopDirectives: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also add some comment here explaining what this config does.

comment.text.match(CommentConfig::COMMENT_DIRECTIVE_REGEXP)
end

def source_without_directive(line)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be named line_length_without_directive or something like this?

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 8, 2016

Not sure if # rubocop:[dis]able type comments already have a name. In the source they seem to be called directives, so I went with that when naming the option IgnoreCopDirectives

The name you chose is OK.

I tried to run the tests/rubocop in Rubinius but all I got was seg faults. I have never tried jruby. CI has me covered?

Yep. Frankly we don't pay much attention to Rubinius support - there are always some issues with it.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 8, 2016

And you'll have to rebase.

The IgnoreCopDirectives option of Metrics/LineLength ignores directives like
'# rubocop:disable ...' when calculating the line's length.
@jmks jmks force-pushed the cop_directives_ignorable_for_line_lengths branch from d4db8ec to b91ea0f Compare October 11, 2016 16:35
@jmks
Copy link
Contributor Author

jmks commented Oct 11, 2016

Updated from the review and rebased.

@bbatsov bbatsov merged commit 6a3442b into rubocop:master Oct 11, 2016
@twe4ked
Copy link
Contributor

twe4ked commented Oct 11, 2016

Thanks very much for your work on this @jmks!

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.

None yet

3 participants