-
Notifications
You must be signed in to change notification settings - Fork 25
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 Rails/HttpPositionalArguments offenses #957
Conversation
Autocorrects all these offenses by running ``` rubocop -f html -o rubocop.html --only Rails/HttpPositionalArguments -a ```
Enables Rails/HttpPositionalArguments (plus other incidental changes)
Removes the monkey-patch added during the Rails 5.2 upgrade in order to avoid kwarg deprecation warnings
@JoeCohen If this removes the use of I believe that fixing the kwarg issue for 2.7 will require further changes to functional_test_case methods, since they do not send the args to Rails |
Thanks @nimmolo! |
Deletes two methods which, due to prior commits in this branch, are no longer used. Responds to #957 (comment)
I'll get to work on the Codeclimate/RuboCop offenses. I'm not sure why there are so many of them. |
@JoeCohen I think it's that your line length in VS Code is not set to 80? I am happy to open and save the file, that will fix it. Also -- I believe I solved the kwargs issue once and for all. Pushed it to this branch. Also -- fixed a rubocop "Prefer |
Hi @JoeCohen - Here are the configs to set up rubocop with VS Code, in case you need them |
There's one more kwarg thing to fix, in session_extensions.rb:104. Commit coming soon. |
Thank you again!
- HTML>Format: Wrap Line Length was set to 120.
- I can use the Ruby > Rubocop settings. But I will leave that for
another day.
- The latest version of RuboCop does some line length autocorrection.
We're on an old version. I'll update us after I get done with this PR.
…On Sat, May 28, 2022 at 4:27 PM andrew nimmo ***@***.***> wrote:
Hi @JoeCohen <https://github.com/JoeCohen> -
I was mistaken, Rubocop doesn't do line length.
It's another setting: hit command-, for settings, then search for line
length. You'll find it under HTML>Format: Wrap Line Length
Here are the configs to set up rubocop with VS Code, in case you need them
[image: Screen Shot 2022-05-28 at 4 15 49 PM]
<https://user-images.githubusercontent.com/1948095/170845823-5fa9a999-923e-4979-a6f5-f0f17c6b07d9.png>
.
—
Reply to this email directly, view it on GitHub
<#957 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALDFBNTCLFEHBSZP7GC23VMKTUJANCNFSM5XHGQSXQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Fixes remaining kwarg deprecations in session_extensions and controller_extensions. Removes &block from args in functional_test_case (not used) Fixes ApiControllerTest and Api2ControllerTest passing hashes instead of kwargs
@JoeCohen I believe this is now good to go. With respect to our calls to http See the rails code here, scroll down to the |
Thanks.
I'll have to look at it in about an hour. I'm being called to dinner now.
…On Sat, May 28, 2022 at 5:44 PM andrew nimmo ***@***.***> wrote:
@JoeCohen <https://github.com/JoeCohen> I believe this is now good to go.
The "Rails/HttpPositionalArguments" cop is incorrect, as far as I can
understand, with respect to our calls to http get etc.
See the rails code here
<https://github.com/rails/rails/blob/aa7c4179ff3b0dba6c14e45cecaf853ac9446d41/actionpack/lib/action_controller/test_case.rb>.
It seems to me it expects it like we're serving it.
—
Reply to this email directly, view it on GitHub
<#957 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALDFD35LGNSKQGO2TOJATVMK4YBANCNFSM5XHGQSXQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I'm not at home but thought of another consideration. When a test inherits from functional_test_case, which the tests that inherit from controller_extensions and session_extensions may or may not do, MO intercepts all calls to |
…rver/mushroom-observer into joe-fix-test-kwargs # Conflicts: # test/controllers/account_controller_test.rb # test/controllers/api_controller_test.rb # test/controllers/collection_number_controller_test.rb # test/controllers/image_controller_test.rb # test/controllers/interest_controller_test.rb # test/controllers/location_controller_test.rb # test/controllers/naming_controller_test.rb # test/controllers/project_controller_test.rb # test/controllers/translation_controller_test.rb
Urk, that was quite a flurry of emails in this thread!! I didn't take the
time to read and understand everything. Is there anything that I should be
particularly aware of??
…On Sat, May 28, 2022 at 10:35 PM codeclimate[bot] ***@***.***> wrote:
Code Climate has analyzed commit c0329e9
<c0329e9>
and detected *5 issues* on this pull request.
Here's the issue category breakdown:
Category Count
Bug Risk 5
View more on Code Climate
<https://codeclimate.com/github/MushroomObserver/mushroom-observer/pull/957>
.
—
Reply to this email directly, view it on GitHub
<#957 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYTNNIOI26ZNS3J73J4HRTVMLJXVANCNFSM5XHGQSXQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
@pellaea - This PR fixes dozens of HttpPositionalArguments offenses, except for 5 in controller_extensions and session_extensions. I believe the cop is throwing a false positive in those. The native Rails test_case method |
PS - in case you're reading on email and didn't catch the typo i edited after sending, the native Rails method is of course |
@pellaea: There's nothing that you should be particularly aware of.
We (i.e., 99% Andrew) are on top of things, and all the changes are in
test, so they can't break anything in production.
@nimmolo: I'd like to wait on merging this, so I will make it a Draft PR to
prevent accidental merger.
In particular, I want to see if updating RuboCop will fix the false
positive HttpPositionalArguments offenses.
My plan:
- New branch.
- Review the default protocol for updating RuboCop
- Update our RuboCop gems (rubocop, rubocop_rails, rubocop_performance)
- Update RuboCop configuration.
- Update rubocop.todo
- Push the branch
- If everything passes and it's OK with you, merge it to master
- Merge master to Fix Rails/HttpPositionalArguments offenses #957
- Rev
…On Sun, May 29, 2022 at 3:48 AM andrew nimmo ***@***.***> wrote:
PS - in case you're reading on email and didn't catch the typo i edited
after sending, the native Rails method is of course get(action, **args)
not get(action: **args). It is used like get(:show, params)
—
Reply to this email directly, view it on GitHub
<#957 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALDFBGUO7PFXFMACKJLMLVMNDRLANCNFSM5XHGQSXQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I've resolved the conflicts locally, but am getting 18 errors. |
Oh wait. I think I messed up the merger. I will try to fix it, then push if successful. |
# Conflicts: # .rubocop_todo.yml # test/controllers/account_controller_test.rb
I'm done for tonight. |
Sounds good to me. Great work!
…On Mon, May 30, 2022 at 12:18 AM Joseph D. Cohen ***@***.***> wrote:
I'm done for tonight.
I'm fine with merging this.
—
Reply to this email directly, view it on GitHub
<#957 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYTNNLIDFR3V2433ZFKPH3VMQ6RTANCNFSM5XHGQSXQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Code Climate has analyzed commit 9021408 and detected 0 issues on this pull request. View more on Code Climate. |
nothing urgent. The 5 stickers are in controller extwnsions and session extensions. I think we can resolve later
… On May 28, 2022, at 7:52 PM, Jason Hollinger ***@***.***> wrote:
Urk, that was quite a flurry of emails in this thread!! I didn't take the
time to read and understand everything. Is there anything that I should be
particularly aware of??
On Sat, May 28, 2022 at 10:35 PM codeclimate[bot] ***@***.***>
wrote:
> Code Climate has analyzed commit c0329e9
> <c0329e9>
> and detected *5 issues* on this pull request.
>
> Here's the issue category breakdown:
> Category Count
> Bug Risk 5
>
> View more on Code Climate
> <https://codeclimate.com/github/MushroomObserver/mushroom-observer/pull/957>
> .
>
> —
> Reply to this email directly, view it on GitHub
> <#957 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAYTNNIOI26ZNS3J73J4HRTVMLJXVANCNFSM5XHGQSXQ>
> .
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
This branch fixes all the Rails/HttpPositionalArguments offenses, including these auxiliary changes:
More details at MO Developers, ruby 2.7.6 and rails 6.1
There is no manual test -- all changes are in test, rather than app