Closed kalsan closed 4 years ago
Generally looks great, thank you! :)
There's a couple of things that could be changed, though, if you have the time? (If you don't, no worries, I can make these tweaks myself later)
ThinkingSphinx::Settings
as one of the defaults - that way, we don't need to have the fallback provided when checking if a statement is too long, you can just always refer to the settings value. There's no need to define a separate constant for the value, just have it has part of the DEFAULTS hash.spec/acceptance/connection_spec.rb
would be ideal as well - you can use write_configuration('maximum_statement_length' => ...)
in the spec. I would recommend testing with a shorter maximum, just so we don't need to worry about how the live Sphinx daemon is actually configured. We just want to make sure Thinking Sphinx itself raises the appropriate exception.Hi Pat
This is untested, hope everything works. I've never worked with rspec before so I tried to interpret your code and deduce the required logic for it to work. Please double-check as this might be error-prone! :-)
Cheers!
Thanks for adding all of that in :) I've made a few further tweaks just to clean things up, particularly with that spec, though the core of what you'd supplied was spot on!
So it's been manually merged (which you can see in the list of commits) - greatly appreciate the contribution! 🎉
Pull request for #1178
I hope this fits the "thinking sphinx" way to implement the changes.