Skip to content
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

testscript: suggest misspelled commands #198

Merged
merged 1 commit into from May 5, 2023
Merged

Conversation

Merovius
Copy link
Contributor

@Merovius Merovius commented Feb 3, 2023

If a command is not found, we go through the list of defined commands and check if any of them are sufficiently close to the one used. "Sufficiently close" is defined by having a Damerau-Levenshtein distance of 1, which feels like it hits the sweet spot between usefulness and ease of implementation.

The negation case is still special-cased, as negation is not in the set of defined commands.

Fixes #190

@Merovius
Copy link
Contributor Author

Merovius commented Feb 3, 2023

Hm I just realized that this might create undesirable results for the !cmd case, as it will result in both suggesting ! cmd and cmd. I probably should special-case this even further (which means all the spelling-correction stuff is useless for actually fixing #190 specifically. Still useful, though).

@Merovius
Copy link
Contributor Author

Merovius commented Feb 3, 2023

Okay, second try :) Also over engineered the test a bit and actually found what's arguably a bug in handling invalid UTF-8 by better fuzzing.

@Merovius
Copy link
Contributor Author

@mvdan You offered to review fixes to #190. FWIW I'd totally accept being told I'm overengineering things and re-trying with something simpler.

@ldemailly
Copy link

I propose #215 for the original problem :-) (and I'm ok if that doesn't get merged, though I think it's simple/straightforward, I'll maintain on my fork meanwhile)

Copy link
Collaborator

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

This is quite a bit of added code, but it's useful and you've clearly put effort and polish into it, and I don't think it will cause additional maintenance burden of its own. SGTM with a couple of nits. There are conflicts as well, but I imagine they will be trivial.

internal/textutil/almost_equal.go Outdated Show resolved Hide resolved
testscript/testscript.go Show resolved Hide resolved
@Merovius
Copy link
Contributor Author

Merovius commented May 5, 2023

I feel like there should be a test that the code in testscript actually does what it's supposed to. I'm trying to figure out how the testscript tests work to make that happen.

@mvdan
Copy link
Collaborator

mvdan commented May 5, 2023

Look at files like testscript/testdata/testscript_update_script.txt; we have "meta-testscripts" that themselves set up a testscript and run it, sometimes with a flag, sometimes expecting a failure :) The extra level allows this kind of flexibility.

@Merovius Merovius force-pushed the master branch 2 times, most recently from 6846479 to 1c0aeee Compare May 5, 2023 13:24
@Merovius
Copy link
Contributor Author

Merovius commented May 5, 2023

Added a couple simple tests and renamed the package PTAL

Copy link
Collaborator

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Thanks! Just one last nit about the tests.

testscript/testdata/testscript_notfound.txt Outdated Show resolved Hide resolved
If a command is not found, we go through the list of defined commands
and check if any of them are sufficiently close to the one used.
"Sufficiently close" is defined by having a Damerau-Levenshtein distance
of 1, which feels like it hits the sweet spot between usefulness and
ease of implementation.

The negation case is still special-cased, as negation is not in the set
of defined commands.

Fixes rogpeppe#190
@mvdan mvdan merged commit 5150104 into rogpeppe:master May 5, 2023
6 checks passed
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.

testscript: Accept !cmd (without space) or provide better error
3 participants