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.

Sign up for GitHub

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

Fix most Ruby 2.7 warnings in Active Record 6.0 #38034

Merged
merged 1 commit into from Dec 19, 2019
Merged

Fix most Ruby 2.7 warnings in Active Record 6.0 #38034

merged 1 commit into from Dec 19, 2019

Conversation

casperisfine
Copy link
Contributor

There are still plenty left, but I'd like a CI run at this stage to have a better view.

@simi
Copy link
Contributor

simi commented Dec 19, 2019

ℹ️ I was looking into this as well. A lot of warnings are coming from pg gem. Those are fixed in upstream, but not released yet.

@kamipo
Copy link
Member

kamipo commented Dec 19, 2019

This will mostly break CI on Ruby 2.6 and 2.5.

@casperisfine
Copy link
Contributor Author

This will mostly break CI on Ruby 2.6 and 2.5.

Yup, just saw it. Trying to make both work.

kamipo
kamipo reviewed Dec 19, 2019
View reviewed changes
activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb Outdated
@@ -561,7 +562,7 @@ def column_exists?(column_name, type = nil, options = {})
# t.index([:branch_id, :party_id], unique: true, name: 'by_branch_party')
#
# See {connection.add_index}[rdoc-ref:SchemaStatements#add_index] for details of the options you can use.
def index(column_name, options = {})
def index(column_name, options={})
Copy link
Member

@kamipo kamipo Dec 19, 2019 •
edited

Choose a reason for hiding this comment

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

Are these changes (options = {} to options={}) automatically applied?
Layout/SpaceAroundOperators is temporarily disabled in #36943 since that cop changed to enforce hard alignment too, but originally we prefer options = {} over options={}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's my mistake I'll revert those befor easking for review.

This PR is still a WIP at this stage, sorry.

@casperisfine casperisfine force-pushed the activerecord-ruby-2.7-warnings-6-0-stable branch from b0e5829 to af7a486 Compare December 19, 2019 12:59
@kamipo
Copy link
Member

kamipo commented Dec 19, 2019

Should this be applied to master branch first? Looks like some of change in this PR isn't applied to master yet.

@casperisfine
Copy link
Contributor Author

Looks like some of change in this PR isn't applied to master yet.

Oh ? I had no idea. @rafaelfranca asked me to clear 2.7 warnings on the 6.0 branch I though the master one was cleared already.

Many of these changes are actually cherry picked from your commits on master, but many need some changes to merge properly

@casperisfine casperisfine force-pushed the activerecord-ruby-2.7-warnings-6-0-stable branch from af7a486 to d16e6ba Compare December 19, 2019 13:28
@kamipo
Copy link
Member

kamipo commented Dec 19, 2019

In master branch, still leave the 2.7 warnings especially for *args delegation since fixing that requires RUBY_VERSION >= "2.7" version checking or options = args.extract_options!; if options.empty? checking, so we'd wait the Ruby core team's decision what is a prefered way (after that the Ruby core team has provided ruby2_keywords).

@casperisfine
Copy link
Contributor Author

ruby2_keywords

Yeah, I saw it wasn't used anywhere, so I assumed it was a no-no. Is it the case? In some instances it would simplify the work a lot, especially on 6-0-stable.

@kamipo
Copy link
Member

kamipo commented Dec 19, 2019

I suppose we can use ruby2_keywords now.

@casperisfine casperisfine force-pushed the activerecord-ruby-2.7-warnings-6-0-stable branch from d16e6ba to 657520f Compare December 19, 2019 14:11
@casperisfine
Copy link
Contributor Author

I think this PR should be good to go now. It's big enough already, I'll continue in another one.

@rafaelfranca rafaelfranca merged commit fecccbf into rails:6-0-stable Dec 19, 2019
@casperisfine
Copy link
Contributor Author

casperisfine commented Dec 19, 2019 •
edited

Kinda unrelated, but there's two failures on master:

Failure:
FilterAttributesTest#test_filter_attributes_on_pretty_print_should_handle_[FILTERED]_value_properly [/rails/activerecord/test/cases/filter_attributes_test.rb:127]:
Expected "#<User:0x000055bb847465b8\n id: nil,\n token: \"[FILTERED]\",\n auth_token: \"[FILTERED]\">\n" to include "auth_token: [FILTERED]".

Looks like the pp behavior changed with 2.7, but I don't know if the test should acknowledge that difference, or try to bring the old behavior back.

@rafaelfranca
Copy link
Member

It seems it is just the output that changed, the behavior we care about (the sensitive value being filtered) is the same, so we can just update the tests.

@casperisfine casperisfine mentioned this pull request Dec 19, 2019
Merged
@hnatt
Copy link
Contributor

hnatt commented Mar 23, 2020

@rafaelfranca will these changes be only released with 6.1? I hoped they will make it to 6.0.2.2, but looks like they didn't.

@kaspth
Copy link
Contributor

kaspth commented Mar 23, 2020

6.0.2.2 was a security only release. It’ll be in 6.0.3 or point to the 6-0-stable in your Gemfile to get it now.

kamipo added a commit to kamipo/rails that referenced this pull request Oct 23, 2020
@kamipo
Re-enable Layout/SpaceAroundOperators cop
92ff708 
We prefer space around operators, but `Layout/SpaceAroundOperators` cop
was temporarily disabled in rails#36943 since that cop changed to check
alignment strictly somehow.

In RuboCop 1.0.0, that is fixed by rubocop/rubocop#8906.

Related rails#38034 (comment),
rails#39770 (comment).
@kamipo kamipo mentioned this pull request Oct 23, 2020
Merged
pixeltrix added a commit to pixeltrix/globalize that referenced this pull request May 2, 2021
@pixeltrix
Prefer forwarding shorthand syntax on Ruby 2.7 and later
5473a39 
In rails/rails#38034 the `save` definition in `ActiveRecord::Validations`
was changed from an options hash to keyword arguments to prevent warnings
on Ruby 2.7. To prevent future changes to method signatures causing
issues use the forwarding shorthand syntax available in 2.7 and later.
pixeltrix added a commit to pixeltrix/globalize that referenced this pull request May 2, 2021
@pixeltrix
Prefer forwarding shorthand syntax on Ruby 2.7 and later
4d253dd 
In rails/rails#38034 the `save` definition in `ActiveRecord::Validations`
was changed from an options hash to keyword arguments to prevent warnings
on Ruby 2.7. To prevent future changes to method signatures causing
issues use the forwarding shorthand syntax available in 2.7 and later.

We have to class_eval the method because even though it's inside the
conditional, Ruby < 2.7 still checks the syntax and it raises an error.
pixeltrix added a commit to pixeltrix/globalize that referenced this pull request May 12, 2021
@pixeltrix
Prefer forwarding shorthand syntax on Ruby 2.7 and later
c24766a 
In rails/rails#38034 the `save` definition in `ActiveRecord::Validations`
was changed from an options hash to keyword arguments to prevent warnings
on Ruby 2.7. To prevent future changes to method signatures causing
issues use the forwarding shorthand syntax available in 2.7 and later.

We have to class_eval the method because even though it's inside the
conditional, Ruby < 2.7 still checks the syntax and it raises an error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kamipo kamipo kamipo left review comments

Assignees
No one assigned
Labels
actionpack activerecord activesupport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@casperisfine @simi @kamipo @rafaelfranca @hnatt @kaspth @byroot