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

Remove Lint/InvalidCharacterLiteral cop #4746

Merged
merged 1 commit into from
Sep 24, 2017
Merged

Remove Lint/InvalidCharacterLiteral cop #4746

merged 1 commit into from
Sep 24, 2017

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Sep 16, 2017

It is uncertain whether this cop is actually useful in any case. The
only use case raises a syntax error and the tests for it are pending.

$ echo "puts(? )" | bundle exec rubocop --stdin test.rb
Inspecting 1 file
E

Offenses:

test.rb:1:6: W: invalid character syntax; use ?\s
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
puts(? )
     ^^
test.rb:1:6: E: unexpected token tEH
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
puts(? )
     ^

1 file inspected, 2 offenses detected
$ bundle exec ruby -e "puts(? )"
-e:1: warning: invalid character syntax; use ?\s
-e:1: syntax error, unexpected '?', expecting ')'
puts(? )
      ^

Thoughts?


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).

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Sep 16, 2017

PR should be green once #4703 lands.

@Drenmi
Copy link
Collaborator

Drenmi commented Sep 16, 2017

I think at some point, all Parser diagnostics were simply wrapped in cops. I'm okay with removing this, since it's unclear what it's actually checking for (and checking for that can't be automatically tested it seems.)

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 17, 2017

@jonas054 Any thoughts?

@deivid-rodriguez
Copy link
Contributor Author

Just found an exact duplicate of this, which was closed: https://github.com/bbatsov/rubocop/pull/4086.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 17, 2017

I recall I closed it mostly because of @rrosenblum's comment. Perhaps there's a real problem, but someone else will have to look into this as I don't have the time to do so right now.

@jonas054
Copy link
Collaborator

I think @dorian was right in his comment saying that it's not possible to reach the cop. The errors it checks for are caught by parser (and thus become Lint/Syntax offenses):

ruby -e 'File.write("test.rb", "p(?\t)")'; rubocop -D --only InvalidCharacterLiteral test.rb
Inspecting 1 file
E

Offenses:

test.rb:1:3: W: Lint/Syntax: invalid character syntax; use ?\t
(Using Ruby 2.1 parser; configure using TargetRubyVersion parameter, under AllCops)
p(?	)
  ^^
test.rb:1:3: E: Lint/Syntax: unexpected token tEH
(Using Ruby 2.1 parser; configure using TargetRubyVersion parameter, under AllCops)
p(?	)
  ^

1 file inspected, 2 offenses detected

I recommend we merge this PR.

@deivid-rodriguez
Copy link
Contributor Author

Yeah, looks like the cop is dead code after all. Codeclimate partially confirms that?¿ https://codeclimate.com/github/bbatsov/rubocop/coverage/59bf68f6de6baf0001000455.

@rrosenblum
Copy link
Contributor

I vaguely remember testing the issue that was mentioned. I don't remember all the details of the issue. Given the new information that we have, I agree that we can remove this cop.

CHANGELOG.md Outdated
@@ -7,6 +7,10 @@
* [#4741](https://github.com/bbatsov/rubocop/issues/4741): Make `Style/SafeNavigation` correctly exclude methods called without dot. ([@drenmi][])
* [#4740](https://github.com/bbatsov/rubocop/issues/4740): Make `Lint/RescueWithoutErrorClass` aware of modifier form `rescue`. ([@drenmi][])

### Changes

* [#4746](https://github.com/bbatsov/rubocop/pull/4746): The `Lint/InvalidCharacterLiteral` cop has been removed since it was preventing a warning that we were not sure if it can be emited by parsable ruby. ([@deivid-rodriguez][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess the message here should be simply "because it was never being actually triggered" (or something like this).

@@ -26,6 +26,9 @@ class Config
'`Style/TrailingCommaInArguments` instead.',
'Rails/DefaultScope' =>
'The `Rails/DefaultScope` cop no longer exists.',
'Lint/InvalidCharacterLiteral' =>
'The `Lint/InvalidCharacterLiteral` cop has been removed since we ' \
'are not aware of a valid use case for this cop.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

It is uncertain whether this cop is actually useful in any case. The
only use case raises a syntax error...

```
$ echo "puts(? )" | bundle exec rubocop --stdin test.rb
Inspecting 1 file
E

Offenses:

test.rb:1:6: W: invalid character syntax; use ?\s
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
puts(? )
     ^^
test.rb:1:6: E: unexpected token tEH
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
puts(? )
     ^

1 file inspected, 2 offenses detected
```

```
$ bundle exec ruby -e "puts(? )"
-e:1: warning: invalid character syntax; use ?\s
-e:1: syntax error, unexpected '?', expecting ')'
puts(? )
      ^
```
@deivid-rodriguez
Copy link
Contributor Author

@bbatsov Updated!

@bbatsov bbatsov merged commit 325cfa7 into rubocop:master Sep 24, 2017
@deivid-rodriguez deivid-rodriguez deleted the drop_not_working_cop branch September 24, 2017 08:50
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.

5 participants