thinking-sphinx icon indicating copy to clipboard operation
thinking-sphinx copied to clipboard

Fixes for Rails 7.1

Open jdelStrother opened this issue 2 years ago • 2 comments

This was going to just be a tiny fix for a LogSubscriber deprecation in Rails 7.1 (https://github.com/pat/thinking-sphinx/commit/cde7554aaf5169ea633015b40992204c53953e2d) but I ran into a few other issues getting the test-suite running.

jdelStrother avatar Oct 11 '23 11:10 jdelStrother

@jdelStrother What errors do you get without this fix?

atomical avatar Nov 27 '23 19:11 atomical

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)`).

jdelStrother avatar Nov 28 '23 09:11 jdelStrother

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!

rpheath avatar Jun 13 '24 17:06 rpheath

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.

jdelStrother avatar Jun 13 '24 20:06 jdelStrother

Thanks @jdelStrother — any thoughts @pat?

rpheath avatar Jun 14 '24 15:06 rpheath

I've fixed the existing failures in https://github.com/pat/thinking-sphinx/pull/1263, maybe we can start from there

jdelStrother avatar Jun 15 '24 14:06 jdelStrother

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

pat avatar Jun 16 '24 06:06 pat

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 ?)

jdelStrother avatar Jun 16 '24 09:06 jdelStrother

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

pat avatar Jun 16 '24 11:06 pat

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

rpheath avatar Jun 22 '24 22:06 rpheath

@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. 🤞🏻

pat avatar Jul 07 '24 07:07 pat

@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!

rpheath avatar Jul 11 '24 20:07 rpheath