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 Lint::FormatParameterMismatch cop #8042
Fix Lint::FormatParameterMismatch cop #8042
Conversation
d107590
to
fd6ba6d
Compare
Please let me know if this change isn't welcome. |
Looks like a great PR. I was curious: wouldn't it be best to consider this an offense? Or do you feel it would be better in a different cop? |
Oh, well. I didn't think about this. Yes, it definitely makes sense to add an offense in this case. Regarding adding new cope. It looks reasonable to add a new cope and not to change/expand semantic and responsibility of the current one. On the other hand adding new cope may have some performance impact and lead to code duplication - it will be very similar to the Anyway I would like to add a new offense if it's welcome. |
fd6ba6d
to
189c21d
Compare
Just fixed conflict with master in the changelog file. |
Given the fact that these offenses are going to be extra super rare, that you're already doing all the work, and that this cop is already checking for errors with |
Got it. So what is the best way to add new offense from your point of view? To create a new cop or update this one? |
I would update this one :-) |
189c21d
to
dde8cf5
Compare
Done |
The cop documentation has to be updated as well - now it doesn't mention anywhere this class of problems. |
dde8cf5
to
b8377e9
Compare
Updated both legacy and asciidoc documentation. |
Could anybody please re-run specs on CI? The only failing environment is JRuby. Specs are passing when I run them repeatedly but fails with some particular seed |
I can't wait to set the defaults properly. These error message are completely un-informative "expect(cop.offenses.empty?).to be(false) got true". I don't see a way to restart the CI. Just amend your last commit (a change in the datestamp is sufficient) and force push again... |
b8377e9
to
8f7ca64
Compare
The cop failed to process format string in invalid format and raised exception. There are several cases when format string contains valid segments but these segments aren't compatible. For instance numbered format cannot be mixed with unnumbered or named. Such format string will lead to ArgumentError when code is executed but sometimes it's needed e.g. in tests to check how the code such invalid format strings.
8f7ca64
to
b11f071
Compare
The only checks failed are:
I have no idea what does cause it and not sure whether it's related to my changes in this PR - locally |
I noticed that these checks fail on all the PR merged recently. So I believe that changes in this PR are safe to merge as well. |
Thanks for tackling this! |
The cop failed to process format string in invalid format and raised exception. There are several cases when format string contains valid segments but these segments aren't compatible.
For instance numbered format cannot be mixed with unnumbered or named. Such format string will lead to ArgumentError when code is executed but sometimes it's needed e.g. in tests to check how the code handles such invalid format strings.
Example of such code - tests in RubySpec project
Changes
Lint::FormatParameterMismatch
cope#valid?
method toUtils::FormatString
class to check mixing of formatsExample
Running Rubocop on this code (where numbered format is mixed with unnumbered):
leads to the following error
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.