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 Lint/PercentStringArray and Lint/PercentSymbolArray cops #3165

Merged
merged 1 commit into from
Jun 19, 2016

Conversation

owst
Copy link
Contributor

@owst owst commented May 25, 2016

Adds a new cop Lint/PercentString cop:

  # This cop checks for colons and commas in %i, e.g.
  #
  #   `%i(:foo, :bar)`
  #
  # and quotes and commas in %w, e.g.
  #
  #   `%w('foo', "bar")`
  #
  # in both cases, it is more likely that the additional characters are
  # unintended (for example, mistranslating an array of literals to percent
  # string notation) rather than meant to be part of the resulting
  # strings/symbols.

I recently made this mistake in my work code, and it was only due to a keen-eyed reviewer (@Qlexander - thanks 👍) that I avoided introducing a bug, so I think Rubocop can help us out here.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • 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.

@owst owst force-pushed the add_line_percent_string_cop branch from 9e40a38 to a091b34 Compare May 25, 2016 22:08
@@ -15,6 +15,7 @@ class GlobalVars < Cop

# predefined global variables their English aliases
# http://www.zenspider.com/Languages/Ruby/QuickRef.html
# rubocop:disable Lint/PercentString
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of $, on line 26

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we can account account for cases like these.

@owst owst force-pushed the add_line_percent_string_cop branch 2 times, most recently from c57d2fa to 133fa77 Compare May 26, 2016 13:07
@joehorsnell
Copy link

Looks good @owst - only minor comments would be to a) potentially call it Lint/PercentArrayLiteral since they are literal array rather than string syntax and b) maybe have two linters, one for the string array and the other for the symbol array.

@bbatsov
Copy link
Collaborator

bbatsov commented May 28, 2016

@joehorsnell Great remarks. Probably I'd go with different cops.

@@ -6,7 +6,9 @@
describe RuboCop::Cop::Lint::LiteralInCondition do
subject(:cop) { described_class.new }

# rubocop:disable Lint/PercentString
Copy link
Collaborator

Choose a reason for hiding this comment

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

Guess the cop could account for cases like this one.

@owst
Copy link
Contributor Author

owst commented May 29, 2016

About naming - the literals ruby doc refers to them as Percent Strings

@owst owst force-pushed the add_line_percent_string_cop branch from 133fa77 to 13d0d22 Compare May 29, 2016 21:31
@owst
Copy link
Contributor Author

owst commented May 29, 2016

@bbatsov separate cops - done!

@owst owst changed the title Add new Lint/PercentString cop Add Lint/PercentStringArray and Lint/PercentSymbolArray cops May 29, 2016
@owst owst force-pushed the add_line_percent_string_cop branch 2 times, most recently from 9f433b2 to 11c39bb Compare June 2, 2016 00:24
@owst owst force-pushed the add_line_percent_string_cop branch 2 times, most recently from 89ccfcb to 71b502f Compare June 12, 2016 00:34
@owst owst force-pushed the add_line_percent_string_cop branch from 71b502f to 32c52e4 Compare June 15, 2016 23:57
@bbatsov
Copy link
Collaborator

bbatsov commented Jun 19, 2016

Has to be rebased.

@owst owst force-pushed the add_line_percent_string_cop branch from 32c52e4 to c4c014d Compare June 19, 2016 08:58
@owst
Copy link
Contributor Author

owst commented Jun 19, 2016

@bbatsov - done!

@bbatsov bbatsov merged commit 3406eaa into rubocop:master Jun 19, 2016
Neodelf pushed a commit to Neodelf/rubocop that referenced this pull request Oct 15, 2016
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.

3 participants