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

Address some of the AREL deprecation warnings #2633

Closed
wants to merge 2 commits into from

Conversation

mo-nathan
Copy link
Member

No description provided.

@mo-nathan mo-nathan requested a review from nimmolo January 4, 2025 06:20
@coveralls
Copy link
Collaborator

coveralls commented Jan 4, 2025

Coverage Status

coverage: 93.509%. remained the same
when pulling 2c75f02 on njw-arel-warnings
into 372af50 on main.

@nimmolo
Copy link
Contributor

nimmolo commented Jan 4, 2025

The other method that is throwing a ton of warnings is batch_lookup_all_matches in models/name/taxonomy.rb

@nimmolo
Copy link
Contributor

nimmolo commented Jan 4, 2025

I merged this already into #2621, and took your lead of more closely examining all my methods that were now throwing these warnings, to check for reasons they could make Arel grumpy.

They primarily seemed to come from or clauses or joins where two scopes had incompatible ordering conditions, because of the new default ordering scopes on that PR. I examined the AR and added reorder("") or unscoped to these where it seemed right, and the warnings are down from 1700 to 100.

-    matches = Name.where(search_name: name2) if parse&.author.present?
-    matches = Name.where(text_name: name2) if matches.empty?
+    matches = Name.unscoped.where(search_name: name2) if parse&.author.present?
+    matches = Name.unscoped.where(text_name: name2) if matches.empty?

    Name.unscoped.
      where(Name[:correct_spelling_id].coalesce(Name[:id]).
            in(limited_id_set(ids))).
      pluck(*minimal_name_columns)

Note that help is on the way, there's a PR on arel_extensions gem to stop propagation of these warnings.

@mo-nathan
Copy link
Member Author

I don't understand your explanation. I'll have to dig into it.

@nimmolo nimmolo closed this Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants