[Fix #5887] Remove Lint/SplatKeywordArguments cop
#5892
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rrosenblum
approved these changes
May 16, 2018
koic
force-pushed
the
remove_splat_keyword_arguments
branch
from
5fccf37 to
75a885c
Compare
bbatsov
reviewed
May 16, 2018
CHANGELOG.md
Outdated
There was a problem hiding this comment.
That should be under master.
Closes rubocop#5887. This PR removes `Lint/SplatKeywordArguments` cop. The following is an example from rubocop#5887. ```ruby options = { opt1: 1, opt2: 2 } def method1(kw_args); end def method2(**kw_args); end method1(**options) # This will warn in ruby 2.6 and currently warns in rubocop. method2(**options) # This does not warn in ruby 2.6 and should not warn in rubocop. ``` This cop will occur a false positive in `method2` case. However, it seems that there is still no way to know the method signature of receiver object by static analysis mechanism (I might just don't know it) . What this cop was trying to realize is emulating experimental warnings for Ruby 2.6. And that is something of the future. So this PR will remove this cop until the prospect of existing problem solving. I thought about setting the default to disabled, but it seemed that it was not good to be in a condition that it could be used with reported false positives.
koic
force-pushed
the
remove_splat_keyword_arguments
branch
from
75a885c to
4b91183
Compare
albertchae
added a commit
to hackclub/hcb
that referenced
this pull request
May 6, 2023
## TL;DR I ran the spec suite with `RUBYOPT='-W:deprecated'` and fixed all the issues that seemed like true positives. I propose we set this environment variable in prod for a few days and then grep the logs to find additional cases to fix, since our spec suite is not comprehensive. ## Full details Ruby 3 introduced new rules around keyword arguments. - https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/ - https://github.com/ruby/ruby/blob/v3_0_0/NEWS.md - https://www.fastruby.io/blog/ruby/upgrades/upgrade-ruby-from-2.7-to-3.0.html Originally in Ruby 2.7, we would see deprecation warnings for these such as `warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call` However, it turns out they got turned off in 2.7.2 because people complained they were too noisy - https://www.ruby-lang.org/en/news/2020/10/02/ruby-2-7-2-released/ - https://bugs.ruby-lang.org/issues/17000 - https://discuss.rubyonrails.org/t/new-2-7-3-0-keyword-argument-pain-point/74980 To get the previous deprecation warning behavior, we have to run with `RUBYOPT='-W:deprecated'`. So I ran the spec suite with that environment variable and fixed all the issues (except for a few involving ActiveRecord::Enum which I'm still investigating whether they are false positives). Many of these changes were in spec only but some were in app code too. Because the spec suite isn't comprehensive, I propose we turn on the environment variable in production as well and examine logs after. We could try to intelligently log deprecation warnings to airbrake in the same vein as https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66815 and https://www.bugsnag.com/blog/managing-warnings-in-ruby, but I think these approaches might be overkill for bank. P.S. I thought there might be a rubocop rule that could help us, but apparently it was disabled for too many false positives - rubocop/rubocop#5871 - rubocop/rubocop#5887 - rubocop/rubocop#5892 Gitlab wrote a custom rubocop to fix warnings in their codebase, which I also think is overkill for bank https://about.gitlab.com/blog/2021/02/03/how-we-automatically-fixed-hundreds-of-ruby-2-7-deprecation-warnings/
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #5887.
This PR removes
Lint/SplatKeywordArgumentscop.The following is an example from #5887.
This cop will occur a false positive in
method2case.However, it seems that there is still no way to know the method signature of receiver object by static analysis mechanism (I might just don't know it) .
What this cop was trying to realize is emulating experimental warnings for Ruby 2.6. And that is something of the future. So this PR will remove this cop until the prospect of existing problem solving.
I thought about setting the default to disabled, but it seemed that it was not good to be in a condition that it could be used with reported false positives.
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).and description in grammatically correct, complete sentences.
rake defaultorrake parallel. It executes all tests and RuboCop for itself, and generates the documentation.