Add new Lint/SplatKeywordArguments cop
#5871
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
koic
force-pushed
the
add_new_lint_splat_keyword_arguments_cop
branch
from
58f9e1b to
cf37c88
Compare
## Summary This cop emulates the following Ruby warnings in Ruby 2.6. ruby/ruby@a23eca2 ```console % ruby -we "def m(a) end; h = {foo: 1}; m(**h)" -e:1: warning: passing splat keyword arguments as a single Hash to `m' ``` This cop does not have autocorrect because uses of splat keyword arguments duplicates the argument hash instance. So, it is not necessarily compatible code. ### Use splat keyword arguments (`foo(**h)`) ```console % ruby -vwe "def m(a) p a.object_id; end; h = {k: :v}; p h.object_id; m(**h)" ruby 2.6.0dev (2018-05-08 trunk 63359) [x86_64-darwin17] 70248917856620 -e:1: warning: passing splat keyword arguments as a single Hash to `m' 70248917856460 ``` ### Use non-splat keyword arguments (`foo(h)`) ```console % ruby -vwe "def m(a) p a.object_id; end; h = {k: :v}; p h.object_id; m(h)" ruby 2.6.0dev (2018-05-08 trunk 63359) [x86_64-darwin17] 70211441327220 70211441327220 ``` ## Other Information While this warning is experimental, however work on rails/rails has begun. - rails/rails#32447 - rails/rails#32556 - rails/rails#32612
koic
force-pushed
the
add_new_lint_splat_keyword_arguments_cop
branch
from
cf37c88 to
cb073c2
Compare
|
Thanks! 👍 |
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
2 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.
Summary
This cop emulates the following Ruby warnings in Ruby 2.6.
ruby/ruby@a23eca2
This cop does not have autocorrect because uses of splat keyword arguments duplicates the argument hash instance. So, it is not necessarily compatible code.
Use splat keyword arguments (
foo(**h))Use non-splat keyword arguments (
foo(h))Other Information
While this warning is experimental, however work on rails/rails has begun.
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.