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 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][])

This comment has been minimized.

Copy link
@bbatsov

bbatsov Oct 8, 2016

Collaborator

This should be under "New Features".

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

This comment has been minimized.

Copy link
@bbatsov

bbatsov Oct 8, 2016

Collaborator

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)

This comment has been minimized.

Copy link
@bbatsov

bbatsov Oct 8, 2016

Collaborator

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

@bbatsov

This comment has been minimized.

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

This comment has been minimized.

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 jmks:cop_directives_ignorable_for_line_lengths branch from d4db8ec to b91ea0f Oct 11, 2016
@jmks

This comment has been minimized.

Copy link
Contributor Author

jmks commented Oct 11, 2016

Updated from the review and rebased.

@bbatsov bbatsov merged commit 6a3442b into rubocop-hq:master Oct 11, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@twe4ked

This comment has been minimized.

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.