Skip to content

fix: GlobDistance handles / and * correctly#281

Open
letFunny wants to merge 1 commit intocanonical:mainfrom
letFunny:bug-fix-globdistance-slash
Open

fix: GlobDistance handles / and * correctly#281
letFunny wants to merge 1 commit intocanonical:mainfrom
letFunny:bug-fix-globdistance-slash

Conversation

@letFunny
Copy link
Copy Markdown
Collaborator

@letFunny letFunny commented Apr 2, 2026

  • Have you signed the CLA?

As in the other PR, I stumbled upon this bug while optimizing the algorithm. I need the fix to be in before continuing the work on performance.

Fixes the long-standing bug in #63.

@letFunny letFunny added the Bug An undesired feature ;-) label Apr 2, 2026
Copy link
Copy Markdown
Collaborator

@upils upils left a comment

Choose a reason for hiding this comment

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

Thanks @letFunny, nice little fix. I suggested a couple non-blocking changes.

return Cost{SwapAB: Inhibit, DeleteA: Inhibit, InsertB: Inhibit}
}
if ar == '*' || br == '*' {
if ar == '*' && br == '/' {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This may be a matter of taste but we could get rid of a level of indentation with:

if ar == '*' && br == '/' {
    return Cost{SwapAB: Inhibit, DeleteA: 0, InsertB: Inhibit}
}
if ar == '/' && br == '*' {
    return Cost{SwapAB: Inhibit, DeleteA: Inhibit, InsertB: 0}
}
if ar == '*' || br == '*' {
    return Cost{SwapAB: 0, DeleteA: 0, InsertB: 0}
}

{f: strdist.GlobCost, r: 1, a: "a**f/hij", b: "abc/def/hik"},
{f: strdist.GlobCost, r: 2, a: "a**fg", b: "abc/def/hik"},
{f: strdist.GlobCost, r: 0, a: "a**f/hij/klm", b: "abc/d**m"},
{f: strdist.GlobCost, r: 0, a: "/*foo/", b: "/foo/"},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Out of consistency with other tests:

Suggested change
{f: strdist.GlobCost, r: 0, a: "/*foo/", b: "/foo/"},
{f: strdist.GlobCost, r: 0, a: "/*a", b: "/a"},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug An undesired feature ;-)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants