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

Fixes for Rails 7.1 #1252

Merged
merged 5 commits into from
Jun 16, 2024
Merged

Conversation

jdelStrother
Copy link
Contributor

This was going to just be a tiny fix for a LogSubscriber deprecation in Rails 7.1 (cde7554) but I ran into a few other issues getting the test-suite running.

@atomical
Copy link

@jdelStrother What errors do you get without this fix?

@jdelStrother
Copy link
Contributor Author

Fix kwarg expectations with new rspec fixes, eg,

  1) ThinkingSphinx.count passes through the given query and options
     Failure/Error:
       expect(ThinkingSphinx::Search).to receive(:new).with('foo', :bar => :baz).
         and_return(search)

     ArgumentError:
       wrong number of arguments (given 1, expected 0)
     # ./.devenv/bundle/ruby/3.1.0/gems/activesupport-7.1.0/lib/active_support/core_ext/object/with.rb:24:in `with'
     # ./spec/thinking_sphinx_spec.rb:19:in `block (3 levels) in <top (required)>'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:115:in `block in run'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:104:in `loop'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:104:in `run'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:33:in `block (2 levels) in setup'

Fix FilterReflection with Rails 7.1 fixes, eg,

  5) specifying SQL for index definitions concatenates references that have column
     Failure/Error: ReflectionGenerator.new(reflection, name, class_name).call

     NoMethodError:
       undefined method `new' for nil:NilClass

           ReflectionGenerator.new(reflection, name, class_name).call
                              ^^^^
     # ./lib/thinking_sphinx/active_record/filter_reflection.rb:16:in `call'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:31:in `clone_with'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:21:in `block in append_reflections'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:18:in `each'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:18:in `append_reflections'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:9:in `morph!'
     # ./lib/thinking_sphinx/active_record/sql_source.rb:160:in `each'
     # ./lib/thinking_sphinx/active_record/sql_source.rb:160:in `prepare_for_render'
     # ./lib/thinking_sphinx/active_record/sql_source.rb:83:in `render'
     # ./.devenv/bundle/ruby/3.1.0/gems/riddle-2.4.3/lib/riddle/configuration/index.rb:30:in `block in render'
     # ./.devenv/bundle/ruby/3.1.0/gems/riddle-2.4.3/lib/riddle/configuration/index.rb:30:in `collect'
     # ./.devenv/bundle/ruby/3.1.0/gems/riddle-2.4.3/lib/riddle/configuration/index.rb:30:in `render'
     # ./lib/thinking_sphinx/core/index.rb:74:in `render'
     # ./spec/acceptance/specifying_sql_spec.rb:139:in `block (2 levels) in <top (required)>'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:115:in `block in run'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:104:in `loop'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:104:in `run'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:33:in `block (2 levels) in setup'

Fix a LogSubscriber deprecation in Rails 7.1 prevents this deprecation warning:

DEPRECATION WARNING: Bolding log text with a positional boolean is deprecated and will be removed in Rails 7.2. Use an option hash instead (eg. `color("my text", :red, bold: true)`).

@rpheath
Copy link

rpheath commented Jun 13, 2024

Hey @jdelStrother I'd love that fix for the LogSubscriber, it's definitely cluttering up the test output in my application. Looks like this PR stalled out on momentum, any chance of finalizing it and/or isolating that LogSubscriber fix so at least that gets released? Thanks!

@jdelStrother
Copy link
Contributor Author

It doesn't introduce any new test failures (all the test failures are on ruby 2.7 or sphinx, which are already failing on the main branch), so the branch is kind of 'finished' from that point of view.
If @pat's interested in merging new stuff I'll take a look at fixing the remaining test failures, though I wonder if our energy would be better spent just removing the ruby 2.7 & sphinx tests. Certainly from my POV - I'm on an M1 mac, and both those dependencies are pretty problematic on aarch64 last time I checked.

@rpheath
Copy link

rpheath commented Jun 14, 2024

Thanks @jdelStrother — any thoughts @pat?

@jdelStrother jdelStrother force-pushed the logsubscriber-deprecations branch from 3fa7843 to 7838538 Compare June 15, 2024 14:13
@jdelStrother
Copy link
Contributor Author

I've fixed the existing failures in #1263, maybe we can start from there

@jdelStrother jdelStrother force-pushed the logsubscriber-deprecations branch from 7838538 to 4124c15 Compare June 15, 2024 15:37
@pat
Copy link
Owner

pat commented Jun 16, 2024

Would love to get this merged in - @jdelStrother are you able to rebase? (I can sort it out otherwise)

@jdelStrother jdelStrother force-pushed the logsubscriber-deprecations branch from 4124c15 to b541835 Compare June 16, 2024 08:47
@jdelStrother
Copy link
Contributor Author

Hmm, some timeouts in the tests. I’m surprised they’re that slow - want to try re-running them, or I can raise the timeout?
(Do we still want to be running tests against rails 5.0 & 5.1 ?)

@pat
Copy link
Owner

pat commented Jun 16, 2024

Re-running them did the trick - it's rare that a full run passes in a single go.

@pat pat merged commit 8ced2ef into pat:develop Jun 16, 2024
133 checks passed
@rpheath
Copy link

rpheath commented Jun 22, 2024

@pat great to see this one merged! Curious, what's the workflow for getting a new gem published? Thanks again!

@pat
Copy link
Owner

pat commented Jul 7, 2024

@rpheath it's really dependant on my spare time for OSS, which I feel is much less these days :(

Still, trying to get a release out today. I'm just reviewing open PRs, merging what I feel is worthwhile, seeing if there's any open issues I can also offer fixes for, and then it's just a matter of waiting for CI to catch up, writing up the changelog, and running rake release. 🤞🏻

@rpheath
Copy link

rpheath commented Jul 11, 2024

@pat hey no worries! I get it man, completely. I've been a long-time user of this code, it's incredible that you're still willing to put in the time to help all of us out. Thank you!

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.

4 participants