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.

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

Deprecate passing rewhere to merge #45498

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

HParker
Copy link
Contributor

@HParker HParker commented Jun 30, 2022

In 6.1 rewhere was added to merge in #39250 to get more consistent merge behavior.

Sometimes maintaining both conditions on merge calls was removed as default behavior in 7.0: 515aa1e

the release notes,

# Rails 6.1 with rewhere to migrate to Rails 7.0's behavior

and the original deprecation warning,

https://github.com/rails/rails/blob/6-1-stable/activerecord/lib/active_record/relation/where_clause.rb#L169-L173

both show rewhere: true as a way to migrate your 6.1 app to 7.0.

Now that 7.0 is released we can deprecate rewhere as an option on merge and eventually remove the non-rewhere version of merge.

Comment on lines 51 to 53
Setting `rewhere: false` is deprecated and will be removed in Rails 7.2
in Rails 7.2 merge will no longer maintain both conditions,
and will be consistently replaced by the latter condition.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Setting `rewhere: false` is deprecated and will be removed in Rails 7.2
in Rails 7.2 merge will no longer maintain both conditions,
and will be consistently replaced by the latter condition.
`Relation#merge(rewhere: false)` is deprecated without replacement,
and will be removed in Rails 7.2

The proposed message would've looked quite odd after squashing ("in Rails 7.2 in Rails 7.2").

The best thing a deprecation message can communicate is not why it's changing, but what the user needs to do to respond... in this case, we don't have a good answer beyond "figure it out for yourself"; that's not generally an ideal thing to dump onto the user, but in this case we're not expecting people to actually be using this option at all, so a more abrupt "this is going to stop working, soz" seems acceptable.

IMO we don't need to try to fit a "why this is better" into a handful of words here, because 1) the user doesn't care that it's better in principle: what they have is working for them, and 2) the user knows what the option does, because they recently went out of their way to adopt it.

Choose a reason for hiding this comment

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

Isn't #and a possible alternative here?

Comment on lines 46 to 47
Setting `rewhere: true` is deprecated. `rewhere: true` is the default behavior since Rails 7.0.
This option is no longer needed and will be removed in Rails 7.2
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Setting `rewhere: true` is deprecated. `rewhere: true` is the default behavior since Rails 7.0.
This option is no longer needed and will be removed in Rails 7.2
Specifying `Relation#merge(rewhere: true)` is deprecated, as that has now been
the default since Rails 7.0. Setting the rewhere option will error in Rails 7.2

A lot of deprecation messages end in some form of "will be removed in Rails x.y", for good reason, but sometimes I think there are better ways of establishing the same "you need to act before your next upgrade".

@HParker HParker force-pushed the deprecate-rewhere-on-merge branch 2 times, most recently from 523c60d to d8294b2 Compare July 6, 2022 16:13
@HParker
Copy link
Contributor Author

HParker commented Jul 6, 2022

Thanks @matthewd! I updated both messages and tried to make them more similar while I was there.

for the rewhere: false case I went with,

Specifying `Relation#merge(rewhere: false)` is deprecated and will be removed in Rails 7.2.
Remove `rewhere: false` and assure your code expects merge to overwrite conditions.

I think we could still add more information, but I am relying on the fact that if you are using rewhere on merge you probably at least know what rewhere on merge did before. Alternatively we can add to the docs and link it here. I am happy to do that work if you think it is necessary. I don't believe there is a good section of documentation to link to today.

for rewhere: true

Specifying `Relation#merge(rewhere: true)` is deprecated, as that has now been
the default since Rails 7.0. You can remove this option and behavior will not change.
The `rewhere:` option will be removed in Rails 7.2.

I adopted your message almost exactly, except I said the option is removed instead of the option will error since I am not confident it will error unless we purposefully make it error.

I don't feel strongly, about either message, so I am happy to adjust them again!

@HParker HParker force-pushed the deprecate-rewhere-on-merge branch 2 times, most recently from 4c52937 to 3c580d3 Compare July 11, 2022 23:14
In 6.1 rewhere was added to `merge` in rails#39250 to get more consistent `merge` behavior.

Sometimes maintaining both conditions on `merge` calls was removed as default behavior in 7.0: rails@515aa1e

the release notes,

https://github.com/rails/rails/blob/630342e3203c0414f834de43832c77e4a1315172/guides/source/7_0_release_notes.md?plain=1#L211

and the original deprecation warning,

https://github.com/rails/rails/blob/6-1-stable/activerecord/lib/active_record/relation/where_clause.rb#L169-L173

both show `rewhere: true` as a way to migrate your 6.1 app to 7.0.

Now that 7.0 is released we can deprecate `rewhere` as an option on merge and eventually remove the non-rewhere version of `merge`.
@HParker HParker force-pushed the deprecate-rewhere-on-merge branch from 3c580d3 to 12d6d98 Compare July 12, 2022 16:31
@HParker
Copy link
Contributor Author

HParker commented Jul 12, 2022

On second thought I like @matthewd's wording a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants