Skip to content

Add levenshtein distance. Fix #404 #567

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add levenshtein distance. Fix #404 #567

wants to merge 1 commit into from

Conversation

vpinna80
Copy link
Collaborator

REOPENED DUE TO WRONG MERGE

  • Add levenshtein to grammar

  • Ignore build folder

  • Add doc for levenshtein operator

  • Add levenshtein tests

@vpinna80
Copy link
Collaborator Author

vpinna80 commented Apr 14, 2025

@NicoLaval Please read: #558 (comment)

You can use this branch (rebased on master) for further editing.

@vpinna80
Copy link
Collaborator Author

Comments copied from another thread:

@linardian said:

Good afternoon.
As decided in the last TF meeting, for this issue it will be defined a new operator with the possibility to indicate which string distance measure is going to be applied, something like:

string_distance(method, string1, string2)

with “method” having values in (levenshtein,….).
So please proceed along this direction.
Thank you

@NicoLaval said:

Good afternoon. As decided in the last TF meeting, for this issue it will be defined a new operator with the possibility to indicate which string distance measure is going to be applied, something like: string_distance(method, string1, string2) with “method” having values in (levenshtein,….). So please proceed along this direction. Thank you

Hi, I think it's harder because number of params is not fixed, so it seems to be easier to name directly levenshtein as an operator for the 2.2 version (and go deeper in this param issue for 3.0). Do you agree?

@linardian said:

No, sorry but I do not agree. As Franck remarked, we can start with two options (Levenshtein and another one), so the syntax won’t change and is more flexible to add more string distance measures in the next releases.

@antonio-olleros said:

venshtein and another one), so the syntax won’t change and is more flexible to add more string distance measures in the next releases.

Hi! Wouldn't it be easier to leave the method as third parameter, and optional with a default?

@linardian
Copy link
Collaborator

Ok for me. I just did not want to add just a Levenshtein operator, so the syntax:
string_distance(string1, string2, method)
with the "method" parameter optional with default is fine for me.

@NicoLaval
Copy link
Collaborator

string_distance(method, string1, string2)

Hello @linardian,

As number of parameter is not fix, isn't it better to have string_distance(method, string1, string2, ...) with the 3 first parameters mandatory?

I will update at the end of the week (quick holidays this week).

@linardian
Copy link
Collaborator

Anyway this fix will go in the 2.2 version... I will shortly copy alla the documentation in the new folder. Only fixes will be allowed in the 2.1 release.

@antonio-olleros
Copy link
Contributor

string_distance(method, string1, string2)

Hello @linardian,

As number of parameter is not fix, isn't it better to have string_distance(method, string1, string2, ...) with the 3 first parameters mandatory?

I will update at the end of the week (quick holidays this week).

Hi! I think it is fixed, we can only compare two strings. right? So the proposal for me would be string1 and string2 as mandatory, method as optional, so a maximum of 3

@linardian
Copy link
Collaborator

@nico, since we have copied all files to 2.2, please re-apply your changes to the new folder. Thanks!

Copy link
Collaborator

@linardian linardian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said, the operator (string_distance or similar) will have 3 parameters: string1, string2 and (optional) a method (default is Levenshtein). Please take input from 2.2 folder.

* Add levenshtein to grammar

* Ignore build folder

* Add doc for levenshtein operator

* Add levenshtein tests
@vpinna80
Copy link
Collaborator Author

Hi @NicoLaval,
I have rebased the levenshtein branch on the master's head.
Please use this to do the changes requested by @linardian .

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.

4 participants