Skip to content

Conversation

@bgilhome
Copy link

@bgilhome bgilhome commented Mar 6, 2020

This allows the auto-fuzziness format 'AUTO:,' documented at https://www.elastic.co/guide/en/elasticsearch/reference/7.6/common-options.html#fuzziness.

@pounard
Copy link
Member

pounard commented Mar 11, 2020

Hello ! Thank you for the pull request.

I'll have a look as soon as I can. Considering this does some if condition magic, I'd like to add unit tests along with it, to ensure there's no regression, so when I'll have one hour or two to spare, I'll starting writing those tests.

Then we'll merge your PR if everything goes fine :)

@pounard
Copy link
Member

pounard commented Nov 17, 2021

That's a lot of string manipulation, I guess there's probably a cleaner way to achieve this.

Plus, I see that in this spec that you can give LOW and HIGH values, I'd prefer to have those as explicit method parameters.

But since that this method is not supposed to change its behaviour (we want to keep backward compatibility) I'd prefer a solution such as adding a new method instead, i.e.:

setAutoFuzyness($low = null, $high = null) {
  // Validate int, raise exception if not, set values into the class.
}

Then add explicit $autoFuzynessLow and $autoFuzynessHigh as class properties.

In all cases, this method must be documented accordingly, and the setFuzyness() method must also document this method exists (and deprecate the AUTO being a parameter, fallback on setAutoFuzyness() when detected).

I am under the impression that LOW and HIGH values are from Elastic dialect or is this in Lucene syntax standard ? If it's part of Elastic dialect, it must be documented as such. I'm sorry, I didn't to any Lucene related stuff in a while, so my memory fails me, I might just have missed that part when I wrote this API.

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