sloum/bombadillo
sloum
/
bombadillo
5
27
Fork 7
Code Issues 18 Pull Requests 1 Releases 9 Activity

Improved command error messages #189

Merged
sloum merged 12 commits from improve-command-error-messages into release2.3.3 2020-10-24 16:42:20 +00:00
Conversation 4 Commits 12 Files Changed 2 +60 -22
2 changed files with 60 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
client.go
View File

@ -346,21 +346,16 @@ func (c *client) simpleCommand(action string) {
case "HELP", "?":
go c.Visit(helplocation)
default:
c.SetMessage(fmt.Sprintf("Unknown action %q", action), true)
c.SetMessage(syntaxErrorMessage(action), true)
asdf commented 2020-09-11 06:03:32 +00:00
Review
Copy Link

The big change here is that the default case now sends the action to syntaxErrorMessage(). Previously, each action was listed as a specific case, and if the action wasn't valid in that context it would get sent to syntaxErrorMessage().

I can't see an issue with this approach, but feel like it might have been the way it was for a specific reason that I have missed.

The big change here is that the default case now sends the action to `syntaxErrorMessage()`. Previously, each action was listed as a specific case, and if the action wasn't valid in that context it would get sent to `syntaxErrorMessage()`. I can't see an issue with this approach, but feel like it might have been the way it was for a specific reason that I have missed.
sloum commented 2020-09-13 06:18:41 +00:00
Review
Copy Link

Nah, I have been burning the candle at both ends and I'm sure I coded that late at night half asleep. I am 99% sure using a default case should work here. Good improvement 👍

Nah, I have been burning the candle at both ends and I'm sure I coded that late at night half asleep. I am 99% sure using a default case should work here. Good improvement :+1:
c.DrawMessage()
}
}
func (c *client) doCommand(action string, values []string) {
if length := len(values); length != 1 {
c.SetMessage(fmt.Sprintf("Expected 1 argument, received %d", len(values)), true)
c.DrawMessage()
return
}
switch action {
case "CHECK", "C":
case "C", "CHECK":
c.displayConfigValue(values[0])
c.DrawMessage()
case "PURGE", "P":
err := c.Certs.Purge(values[0])
if err != nil {
@ -405,24 +400,22 @@ func (c *client) doCommand(action string, values []string) {
c.saveFile(u, fn)
default:
c.SetMessage(fmt.Sprintf("Unknown action %q", action), true)
c.SetMessage(syntaxErrorMessage(action), true)
c.DrawMessage()
}
}
func (c *client) doCommandAs(action string, values []string) {
if len(values) < 2 {
c.SetMessage(fmt.Sprintf("Expected 2+ arguments, received %d", len(values)), true)
c.DrawMessage()
return
}
if values[0] == "." {
values[0] = c.PageState.History[c.PageState.Position].Location.Full
}
switch action {
case "ADD", "A":
if len(values) < 2 {
sloum commented 2020-09-13 06:24:00 +00:00
Review
Copy Link

I’m not sure what is going on here. Since we perform this action on line 362 it seems like this would be invalid syntax here. Is this being added with the idea that it is a soft failure that would still return some value? If so, I think I would prefer the error message with a syntax guide.

I am reading this on tildegit and the split is a little hard to follow exactly where I am at but I believe this is in the doCommandAs function and check should only be in doCommand as a valid option.

I’m not sure what is going on here. Since we perform this action on line 362 it seems like this would be invalid syntax here. Is this being added with the idea that it is a soft failure that would still return some value? If so, I think I would prefer the error message with a syntax guide. I am reading this on tildegit and the split is a little hard to follow exactly where I am at but I believe this is in the doCommandAs function and check should only be in doCommand as a valid option.
asdf commented 2020-09-13 20:15:53 +00:00
Review
Copy Link

You are correct, thanks for identifying this. This was another leftover from the original branch I wasn't critical enough about, and am guessing it was originally a copy paste error.

It was added in doCommandAs and doLinkCommandAs, it's now removed.

You are correct, thanks for identifying this. This was another leftover from the original branch I wasn't critical enough about, and am guessing it was originally a copy paste error. It was added in doCommandAs and doLinkCommandAs, it's now removed.
c.SetMessage(syntaxErrorMessage(action), true)
c.DrawMessage()
return
}
if values[0] == "." {
values[0] = c.PageState.History[c.PageState.Position].Location.Full
}
msg, err := c.BookMarks.Add(values)
if err != nil {
c.SetMessage(err.Error(), true)
@ -441,8 +434,18 @@ func (c *client) doCommandAs(action string, values []string) {
c.Draw()
}
case "SEARCH":
if len(values) < 2 {
c.SetMessage(syntaxErrorMessage(action), true)
c.DrawMessage()
return
}
c.search(strings.Join(values, " "), "", "")
case "SET", "S":
if len(values) < 2 {
c.SetMessage(syntaxErrorMessage(action), true)
c.DrawMessage()
return
}
if _, ok := c.Options[values[0]]; ok {
val := strings.Join(values[1:], " ")
if !validateOpt(values[0], val) {
@ -473,7 +476,7 @@ func (c *client) doCommandAs(action string, values []string) {
c.SetMessage(fmt.Sprintf("Unable to set %s, it does not exist", values[0]), true)
c.DrawMessage()
default:
c.SetMessage(fmt.Sprintf("Unknown command structure"), true)
c.SetMessage(syntaxErrorMessage(action), true)
c.DrawMessage()
}
}
@ -523,7 +526,7 @@ func (c *client) doLinkCommandAs(action, target string, values []string) {
out = append(out, values...)
c.doCommandAs(action, out)
default:
c.SetMessage(fmt.Sprintf("Unknown command structure"), true)
c.SetMessage(syntaxErrorMessage(action), true)
c.DrawMessage()
}
}
@ -655,7 +658,7 @@ func (c *client) doLinkCommand(action, target string) {
}
c.saveFile(u, fn)
default:
c.SetMessage("Unknown command structure", true)
c.SetMessage(syntaxErrorMessage(action), true)
c.DrawMessage()
}
@ -1203,6 +1206,13 @@ func findAvailableFileName(fpath, fname string) (string, error) {
return savePath, nil
}
func syntaxErrorMessage(action string) string {
if val, ok := ERRS[action]; ok {
return fmt.Sprintf("Incorrect syntax. Try: %s", val)
}
return fmt.Sprintf("Unknown command %q", action)
}
func updateTimeouts(timeoutString string) error {
sec, err := strconv.Atoi(timeoutString)
if err != nil {

28
help.go Normal file
View File

@ -0,0 +1,28 @@
package main
// ERRS maps commands to their syntax error message
sloum commented 2020-09-13 15:55:10 +00:00
Outdated
Review
Copy Link

Rather than help text, I think error message or command description might be better here. Mostly because help text will have a new meaning if the full help module gets used at a later point and this language will keep clear how these are used.

Rather than `help text`, I think `error message` or `command description` might be better here. Mostly because help text will have a new meaning if the full help module gets used at a later point and this language will keep clear how these are used.
asdf commented 2020-09-13 19:57:10 +00:00
Outdated
Review
Copy Link

Great point, I've made this change.

Great point, I've made this change.
var ERRS = map[string]string{
"A": "`a [target] [name...]`",
"ADD": "`add [target] [name...]`",
"D": "`d [bookmark-id]`",
"DELETE": "`delete [bookmark-id]`",
"B": "`b [[bookmark-id]]`",
"BOOKMARKS": "`bookmarks [[bookmark-id]]`",
"C": "`c [link_id]` or `check [setting]`",
"CHECK": "`check [link_id]` or `check [setting]`",
"H": "`h`",
"HOME": "`home`",
"P": "`p [host]`",
"PURGE": "`purge [host]`",
"Q": "`q`",
"QUIT": "`quit`",
"R": "`r`",
"RELOAD": "`reload`",
"SEARCH": "`search [[keyword(s)...]]`",
"S": "`s [setting] [value]`",
"SET": "`set [setting] [value]`",
"W": "`w [target]`",
"WRITE": "`write [target]`",
"?": "`?`",
"HELP": "`help`",
}