Skip to content

Allow low/high distance auto-fuzziness syntax in setFuzzyness().#2

Open
bgilhome wants to merge 3 commits into
makinacorpus:masterfrom
bgilhome:master
Open

Allow low/high distance auto-fuzziness syntax in setFuzzyness().#2
bgilhome wants to merge 3 commits into
makinacorpus:masterfrom
bgilhome:master

Conversation

@bgilhome

@bgilhome bgilhome commented Mar 6, 2020

Copy link
Copy Markdown

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

@pounard

pounard commented Mar 11, 2020

Copy link
Copy Markdown
Member

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

pounard commented Nov 17, 2021

Copy link
Copy Markdown
Member

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