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.

Sign up for GitHub

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

Jump to bottom

testscript: Accept !cmd (without space) or provide better error #190

Closed
awagner-iq opened this issue Dec 7, 2022 · 8 comments · Fixed by #198
Closed

testscript: Accept !cmd (without space) or provide better error #190

awagner-iq opened this issue Dec 7, 2022 · 8 comments · Fixed by #198

Comments

@awagner-iq
Copy link

A colleague is trying out testscript and used !cmd to call a custom command (note the lack of space). They got the error message "!cmd not defined" and where confused why the negation was not recognized. I'd like to improve that.

One option would be to accept the !cmd, parsing it as equivalent to ! cmd. This is easy to implement, but probably not backwards compatible.

Another option is to provide a better error message if a command is not found. e.g. if !cmd is looked up and not found, but cmd is, we could detect that and report "did you mean ! cmd?". Or - harder to implement, but more generally useful - we could search the defined commands by edit-distance to the missed command and if one of them is a close enough match, suggest that. x/tools has an internal package that might help.

I can send a PR, but wanted to discuss what option you prefer.
@ldemailly
Copy link

I hit this as well and I think treating it as ! cmd is the right solution. Why would someone name a binary starting with a ! (which is special in shell as well)

@myitcv
Copy link
Collaborator

myitcv commented Feb 2, 2023

Another option is to provide a better error message if a command is not found

I would tend towards this approach but defer to @rogpeppe and @mvdan

@ldemailly
Copy link

did same typo with !stderr also so I really think the not should work without needing a space

@mvdan
Copy link
Collaborator

mvdan commented Feb 2, 2023

Giving a better error message seems fine to me. I wouldn't want to change the syntax to allow !foo as an alias for ! foo, because that would take our syntax further apart from upstream for little benefit. I think the consistency and readability of the spacing is also nice.

Personally I'd say that erroring on any command with a ! prefix would be fine, because such a command sounds prone to confusion and should be discouraged, and I doubt anyone uses them currently. But if we're really concerned with backwards compatibility, only improving the error when we run into "no such command" also seems fine.

Happy to review a PR :)

@Merovius
Copy link
Contributor

Merovius commented Feb 2, 2023

I think I'll try my hand at a general spelling-correction to improve the error message (which should cover this case as well). If I can't manage that, I'll send a PR to detect !cmd vs. ! cmd specifically.

Merovius added a commit to Merovius/rogpeppe-go-internal that referenced this issue Feb 3, 2023
@Merovius
testscript: suggest misspelled commands
d73317d 
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
@Merovius Merovius mentioned this issue Feb 3, 2023
Merged
Merovius added a commit to Merovius/rogpeppe-go-internal that referenced this issue Feb 3, 2023
@Merovius
testscript: suggest misspelled commands
964bbe9 
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
Merovius added a commit to Merovius/rogpeppe-go-internal that referenced this issue Feb 3, 2023
@Merovius
testscript: suggest misspelled commands
e4df6a0 
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
ldemailly added a commit to fortio/testscript that referenced this issue Mar 29, 2023
@ldemailly
fix rogpeppe#190
320c206
@ldemailly ldemailly mentioned this issue Mar 29, 2023
Merged
@ldemailly
Copy link

ldemailly commented Mar 29, 2023 •
edited
Loading

curious btw

that would take our syntax further apart from upstream

what upstream are you referring to ?

also noticed by the way that lines like

[!exec:false] skip

aren't

[ ! exec:false ] skip

ldemailly added a commit to fortio/testscript that referenced this issue Mar 29, 2023
@ldemailly
locally address rogpeppe#190 (#3)
b2b184e
@mvdan
Copy link
Collaborator

mvdan commented Mar 29, 2023

what upstream are you referring to ?

Currently https://github.com/golang/go/blob/master/src/cmd/go/script_test.go. testscript was started as a copy of that code while being refactored as a library. See all the testscripts upstream has, where the syntax is still very similar.

ldemailly added a commit to fortio/testscript that referenced this issue Mar 29, 2023
@ldemailly
Fix upstream rogpeppe#190: no need for extra spaces when using negation
904407f
@ldemailly ldemailly mentioned this issue Mar 29, 2023
Closed
Merovius added a commit to Merovius/rogpeppe-go-internal that referenced this issue May 5, 2023
@Merovius
testscript: suggest misspelled commands
4a32bb1 
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
Merovius added a commit to Merovius/rogpeppe-go-internal that referenced this issue May 5, 2023
@Merovius
testscript: suggest misspelled commands
6846479 
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
Merovius added a commit to Merovius/rogpeppe-go-internal that referenced this issue May 5, 2023
@Merovius
testscript: suggest misspelled commands
1c0aeee 
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
Merovius added a commit to Merovius/rogpeppe-go-internal that referenced this issue May 5, 2023
@Merovius
testscript: suggest misspelled commands
d899f7f 
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 closed this as completed in #198 May 5, 2023
mvdan pushed a commit that referenced this issue May 5, 2023
@Merovius @mvdan
testscript: suggest misspelled commands
5150104 
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
@thepudds
Copy link
Contributor

thepudds commented May 5, 2023

Hi @Merovius, just wanted to say a quick thanks for doing this!

I hit this just yesterday, and the time between making the mistake to then smacking my head once I realized the mistake was longer than I would care to admit 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@Merovius @myitcv @mvdan @ldemailly @thepudds @awagner-iq