Improved command error messages #189
No reviewers
Labels
No Label
blocked
bug
build
documentation
duplicate
enhancement
finger
gemini
gopher
help wanted
http
in progress
invalid
local
needs-info
non-code
non-functional
non-urgent
question
release
rendering
suggestion
telnet
terminal
urgent
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sloum/bombadillo#189
Loading…
Reference in New Issue
No description provided.
Delete Branch "improve-command-error-messages"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
@ -347,3 +347,3 @@
go c.Visit(helplocation)
default:
c.SetMessage(fmt.Sprintf("Unknown action %q", action), true)
c.SetMessage(syntaxErrorMessage(action), true)
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 tosyntaxErrorMessage()
.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.
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 👍
@ -423,2 +409,3 @@
switch action {
case "C", "CHECK":
c.displayConfigValue(values[0])
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.
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.
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.htmlThe 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!
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?@ -95,3 +95,3 @@
cm.Target = t.val
cm.Type = DOLINK
case Word:
case Word, Action:
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
?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.@ -0,0 +1,28 @@
package main
// ERRS maps commands to their help text
Rather than
help text
, I thinkerror message
orcommand 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.Great point, I've made this change.
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.
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 fordelete
, 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.
Haha just found a small issue.
Using
:help ACTION
will return a parser error instead of the expected syntax error. Sohelp f
shows a syntax error likeIncorrect syntax. Try help
, buthelp add
does not show the syntax error and instead shows the command parser errorFound "ADD" (1), expected value
Next steps in help functionality would include actual support for commands like
:help ACTION
so this will be addressed then.