Skip to content

Make MAXIMUM_STATEMENT_LENGTH customizable as in #1178 #1179

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

Closed
wants to merge 4 commits into from
Closed

Make MAXIMUM_STATEMENT_LENGTH customizable as in #1178 #1179

wants to merge 4 commits into from

Conversation

kalsan
Copy link
Contributor

@kalsan kalsan commented Sep 3, 2020

Pull request for #1178

I hope this fits the "thinking sphinx" way to implement the changes.

Sandro Kalbermatter and others added 2 commits September 3, 2020 10:23
@pat
Copy link
Owner

pat commented Sep 4, 2020

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)

  • Let's get that constant's value shifted into 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.
  • Adding a spec to 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.

@kalsan
Copy link
Contributor Author

kalsan commented Sep 7, 2020

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!

pat added a commit that referenced this pull request Sep 8, 2020
@pat
Copy link
Owner

pat commented Sep 8, 2020

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

@pat pat closed this Sep 8, 2020
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.

2 participants