Improved command error messages #189

asdf · 2020-09-11T05:56:31Z

asdf commented

2020-09-11 05:56:31 +00:00

This is a copy of branch help-integration that updates it and focuses on improving command syntax error messages.

It has a lot of git history including references to help documents that were removed for this version. They are to be re-included once this PR is complete.

This is a copy of branch `help-integration` that updates it and focuses on improving command syntax error messages. It has a lot of git history including references to help documents that were removed for this version. They are to be re-included once this PR is complete.

asdf reviewed 2020-09-11 06:03:32 +00:00

client.go

						
				@ -347,3 +347,3 @@

						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

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

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:

sloum reviewed 2020-09-13 06:23:09 +00:00

sloum reviewed 2020-09-13 06:24:00 +00:00

client.go

						
				@ -423,2 +409,3 @@

					switch action {

					case "C", "CHECK":

						c.displayConfigValue(values[0])

sloum commented

2020-09-13 06:24:00 +00:00

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

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.

sloum commented

2020-09-13 06:28:05 +00:00

I left a couple of comments, but I see a few other things I want to check out. It is late and I need to sleep, lol. I dont think I can focus right now, but I'll check in on this tomorrow.

If you want a laugh check out the branch gemini-html. It adds the worlds worst html parser (it really is a naive joke that I just felt like messing with) to gemini. Allowing people to serve html files over gemini and have a rendered doc appear. You can try it with: https://rawtext.club/~sloum/test/gfu.html

The links wont work as I just copied the document from my html folder and it uses relative links, but they do render correctly. I didnt add table rendering so the table looks like butt.

Not sure that this is a feature I actually want to include/mainline, but it was a fun hour or two.

Thanks for your work on this branch. I look forward to digging in on it tomorrow!

I left a couple of comments, but I see a few other things I want to check out. It is late and I need to sleep, lol. I dont think I can focus right now, but I'll check in on this tomorrow. If you want a laugh check out the branch `gemini-html`. It adds the worlds worst html parser (it really is a naive joke that I just felt like messing with) to gemini. Allowing people to serve html files over gemini and have a rendered doc appear. You can try it with: https://rawtext.club/~sloum/test/gfu.html The links wont work as I just copied the document from my html folder and it uses relative links, but they do render correctly. I didnt add table rendering so the table looks like butt. Not sure that this is a feature I actually want to include/mainline, but it was a fun hour or two. Thanks for your work on this branch. I look forward to digging in on it tomorrow!

sloum reviewed 2020-09-13 15:56:52 +00:00

sloum left a comment

This looks pretty good! I have a few comments and a bit of confusion at a few points (it has been a long time since I dug into any of the parsing; I should have left more comments). I have not been able to checkout this branch. I dont know why... I do git fetch --all and I can see that it grabs your branches, but this one is not listed and I am unable to check it out. Any ideas?

This looks pretty good! I have a few comments and a bit of confusion at a few points (it has been a long time since I dug into any of the parsing; I should have left more comments). I have not been able to checkout this branch. I dont know why... I do `git fetch --all` and I can see that it grabs your branches, but this one is not listed and I am unable to check it out. Any ideas?

cmdparse/parser.go Outdated

						
				@ -95,3 +95,3 @@

						cm.Target = t.val

						cm.Type = DOLINK

					case Word:

					case Word, Action:

sloum commented

2020-09-13 15:53:53 +00:00

Is the idea with adding Action here that we can then do soemthing like :check add? Whereas before that would be a syntax error?

If so, that makes sense. I worry that it opens up other parsing issues. For example, what happens when I try :add quit 7?

Is the idea with adding `Action` here that we can then do soemthing like `:check add`? Whereas before that would be a syntax error? If so, that makes sense. I worry that it opens up other parsing issues. For example, what happens when I try `:add quit 7`?

asdf commented

2020-09-13 19:54:00 +00:00

I think you are right. This must have been required for the command help action to work without it being identified as a parsing error. As this isn't required yet I'll remove it.

I think you are right. This must have been required for the command `help action` to work without it being identified as a parsing error. As this isn't required yet I'll remove it.

help.go Outdated

						
				@ -0,0 +1,28 @@

				package main

				// ERRS maps commands to their help text

sloum commented

2020-09-13 15:55:10 +00:00

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

Great point, I've made this change.

asdf commented

2020-09-13 21:14:06 +00:00

Thanks for the feedback on this. Command parsing is a pretty complex area, thankfully well guarded for errors.

I'm not too sure why you wouldn't be able to fetch the branch - the actual name should be improve-command-error-messages. Originally I did have some issues raising a PR, and in investigating this there was a second branch with a similar name I pushed and then deleted. It has been pushed again with updates, maybe try again and see if it works now?

HTML parsing looks really interesting. As a fun experiment, it looks pretty feasible (but maybe I'm also naive?). Similar support for markdown would be cool, also a reader mode equivalent... Leveraging pandoc like we do lynx to support lots of formats... Anyway, it's pretty cool and seems like it could work well.

Thanks for the feedback on this. Command parsing is a pretty complex area, thankfully well guarded for errors. I'm not too sure why you wouldn't be able to fetch the branch - the actual name should be **improve-command-error-messages**. Originally I did have some issues raising a PR, and in investigating this there was a second branch with a similar name I pushed and then deleted. It has been pushed again with updates, maybe try again and see if it works now? HTML parsing looks really interesting. As a fun experiment, it looks pretty feasible (but maybe I'm also naive?). Similar support for markdown would be cool, also a reader mode equivalent... Leveraging pandoc like we do lynx to support lots of formats... Anyway, it's pretty cool and seems like it could work well.

asdf added the

enhancement

label 2020-09-14 03:45:24 +00:00

sloum commented

2020-09-18 04:33:24 +00:00

I finally got this checked out and built. It works well. I had an initial odd response from something where it looked like delete pulled a generic error message rather than one for delete, but I was unable to replicate the issue. I think this is good to merge in. 👍

Thinking on the HTML, if I built an actual decent parser we could ditch the outside renderers as the default (though still allow them as optional renderers), which would be cool. The parser could work for any html files then (for any protocol). I dont think it is anything I am in a rush to do though and mostly did the initial test version on a lark. Though I'll keep the branch up for folks to play around with and maybe come back to it later.

I fixed the issue with lynx not rendering errors right, btw. Expect a PR tonight or tomorrow.

Feel free to merge this one into the release branch.

I finally got this checked out and built. It works well. I had an initial odd response from something where it looked like `delete` pulled a generic error message rather than one for `delete`, but I was unable to replicate the issue. I think this is good to merge in. 👍 Thinking on the HTML, if I built an actual decent parser we could ditch the outside renderers as the default (though still allow them as optional renderers), which would be cool. The parser could work for any html files then (for any protocol). I dont think it is anything I am in a rush to do though and mostly did the initial test version on a lark. Though I'll keep the branch up for folks to play around with and maybe come back to it later. I fixed the issue with lynx not rendering errors right, btw. Expect a PR tonight or tomorrow. Feel free to merge this one into the release branch.

sloum referenced this issue from a commit

2020-10-24 16:42:19 +00:00

Merge pull request 'Improved command error messages' (#189) from improve-command-error-messages into release2.3.3 Reviewed-on: https://tildegit.org/sloum/bombadillo/pulls/189

sloum merged commit d2e238baac into release2.3.3

2020-10-24 16:42:20 +00:00

asdf commented

2020-10-26 06:04:27 +00:00

Haha just found a small issue.

Using :help ACTION will return a parser error instead of the expected syntax error. So help f shows a syntax error like Incorrect syntax. Try help, but help add does not show the syntax error and instead shows the command parser error Found "ADD" (1), expected value

Next steps in help functionality would include actual support for commands like :help ACTION so this will be addressed then.

Haha just found a small issue. Using `:help ACTION` will return a parser error instead of the expected syntax error. So `help f` shows a syntax error like `Incorrect syntax. Try help`, but `help add` does not show the syntax error and instead shows the command parser error `Found "ADD" (1), expected value` Next steps in help functionality would include actual support for commands like `:help ACTION` so this will be addressed then.

Sign in to join this conversation.

No reviewers