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

Jump to bottom

New cop FormatParamterMismatch checks format for wrong parameter count #2057

Merged
merged 1 commit into from Aug 1, 2015
Merged

New cop FormatParamterMismatch checks format for wrong parameter count #2057

merged 1 commit into from Aug 1, 2015

Conversation

edmz
Copy link
Contributor

@edmz edmz commented Jul 21, 2015

Currently works for format and sprintf only.

@jonas054
Copy link
Collaborator

This is a good idea for a cop, but you're only halfway done. You need to:

  • add tests to cover the new functionality in spec/rubocop/cop/lint/format_parameter_mismatch.rb
  • add default configuration in config/enabled.yml
  • add a note about the new feature in CHANGELOG.md
  • support formatting calls that use the % operator
  • pass RuboCop's self inspection (see the Travis CI log)

@edmz
Copy link
Contributor Author

edmz commented Jul 21, 2015

I agree. @bbatsov was going to comment on it first since I have never done
a cop before.

Thanks for letting me know whsts missing.

On Tuesday, July 21, 2015, Jonas Arvidsson notifications@github.com wrote:

This is a good idea for a cop, but you're only halfway done. You need to:

  • add tests to cover the new functionality in
    spec/rubocop/cop/lint/format_parameter_mismatch.rb
  • add default configuration in config/enabled.yml
  • add a note about the new feature in CHANGELOG.md
  • support formatting calls that use the % operator
  • pass RuboCop's self inspection (see the Travis CI log)

—
Reply to this email directly or view it on GitHub
#2057 (comment).

Eduardo

@edmz
Copy link
Contributor Author

edmz commented Jul 21, 2015

I've made some progress on the tests, but somehow, I cant run 'rake spec' without getting:

rubocop/spec/rubocop/cop/lint/format_parameter_mismatch.rb:5:in `<top (required)>': uninitialized constant RuboCop::Cop::Lint::FormatParameterMismatch (NameError)

Any idea what could be wrong?

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 22, 2015

Likely you didn't add it here - https://github.com/bbatsov/rubocop/blob/master/lib/rubocop.rb

Grepping for the name of some cop is a good way to find all the changes you have to make when introducing a new one.

bbatsov
bbatsov reviewed Jul 22, 2015
View reviewed changes
lib/rubocop/cop/lint/format_parameter_mismatch.rb
module Cop
module Lint
# This lint sees if there is a mismatch between the number of
# expected parameters for format and what is actually passed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This class comment should also include some examples of the kind of code that would trigger the cop.

@edmz
Copy link
Contributor Author

edmz commented Jul 22, 2015

Thanks for the input. I am making progress but I think I hit a roadblock.

Is there a way to access to contents of a const?

This is the case I am concerned about:

A_CONST = "%s %s"
format(A_CONST, 1, 2)

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 22, 2015

That's doable, but it's pretty hard. Our policy so far has been to simply ignore such cases.

@edmz
Copy link
Contributor Author

edmz commented Jul 22, 2015

Ok, sounds good.

@edmz
Copy link
Contributor Author

edmz commented Jul 22, 2015

Please let me know if I am missing anything.

bbatsov
bbatsov reviewed Jul 22, 2015
View reviewed changes
CHANGELOG.md
@@ -2,6 +2,9 @@

## master (unreleased)

* [#2057](https://github.com/bbatsov/rubocop/pull/2057): New cop `Lint/FormatParameterMismatch` checks
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 in "New features". You should add yourself to the bottom of this file.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 22, 2015

Apart from my small remarks the code looks good. Before we can merge the cop you'll also have to squash all commit together and ideally run this on a few bigger projects (e.g. Rails and Rubinius). This helps us be sure there aren't edge cases we missed.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 22, 2015

One more thing - I don't see support for:

puts "%.4s\\%.2s\\%s" % ["1","2","3"]

@edmz
Copy link
Contributor Author

edmz commented Jul 22, 2015

Good catch!

On Wed, Jul 22, 2015 at 12:46 PM Bozhidar Batsov notifications@github.com
wrote:

One more thing - I don't see support for:

puts "%.4s%.2s%s" % ["1","2","3"]

—
Reply to this email directly or view it on GitHub
#2057 (comment).

@edmz
Copy link
Contributor Author

edmz commented Jul 22, 2015

I will try to add more coverage and use cases, but it might be very hard to be exhaustive due to the flexibility of what sprintf() can receive.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 24, 2015

No problem. Take your time.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 27, 2015

@edmz Heads up - I plan to release 0.33 later this week. If you'd like this cop to get in you'll have to wrap it soon.

@edmz
Copy link
Contributor Author

edmz commented Jul 28, 2015

@bbatsov so I worked on this and:

  • squashed everything in 1 commit
  • ran it against rubinius and no crashes
  • added all the format parameters found in sprintf, except for the positional ones, but having a mismatch on those will raise an exception by ruby

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 29, 2015

You'll need to rebase your branch on top of the current master branch.

@edmz
Copy link
Contributor Author

edmz commented Jul 29, 2015

@bbatsov Rebased! I've never done so much git-fu before :)

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 30, 2015

I still see a merge conflict. Guess you'll have to rebase again.

@edmz
Copy link
Contributor Author

edmz commented Jul 30, 2015

Rebased again. Its the changelog. I guess you merged something before merging this that created the conflict. Hopefully this time it will work.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 31, 2015

I've noticed a typo in your commit message.

bbatsov
bbatsov reviewed Jul 31, 2015
View reviewed changes
spec/rubocop/cop/lint/format_parameter_mismatch_spec.rb
expect(cop.offenses).to be_empty
end

it 'registers an offense for %' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, String#% instead of % - it's more informative.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 31, 2015

FormatParamterMismatch -> FormatParameterMismatch

@edmz
Copy link
Contributor Author

edmz commented Jul 31, 2015

Ah, missed that. Hopefully I got it right this time.

On Fri, Jul 31, 2015 at 11:02 AM Bozhidar Batsov notifications@github.com
wrote:

FormatParamterMismatch -> FormatParameterMismatch

—
Reply to this email directly or view it on GitHub
#2057 (comment).

bbatsov added a commit that referenced this pull request Aug 1, 2015
@bbatsov
Merge pull request #2057 from edmz/format_params_mismatch
712fec5 
New cop `FormatParamterMismatch` checks `format` for wrong parameter count
@bbatsov bbatsov merged commit 712fec5 into rubocop:master Aug 1, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 1, 2015

👍 We're good to go! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@edmz @jonas054 @bbatsov @cremno