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

word_parse.py: parse_backslash (errors + hinting questions) #1824

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

bar-g
Copy link
Contributor

@bar-g bar-g commented Feb 8, 2024

This is to provide consistent short error messages, and adds minimal hinting questions for context and immediate help-on-error.

Note how the messages even improve more once parse_backslash gets renamed into strict_backslash_escape.

Example error messaage:

Invalid char escape in "-string literal (parse_backslash, no verbatims)
       Allowed: \", \$, or \\ to denote ", $, or \
       * use \\ to denote a \ ?
       * 'chain'"different"$'literals' ?
       * (ysh) concat "- ++ u'- or b'-literals ?

osh/word_parse.py Outdated Show resolved Hide resolved
@bar-g bar-g changed the title word_parse.py: consistent parse_backslash error + hint word_parse.py: consistent parse_backslash errors + hinting questions Feb 21, 2024
@bar-g
Copy link
Contributor Author

bar-g commented Feb 21, 2024

The error + question style works out quite good. (see description)

@bar-g bar-g changed the title word_parse.py: consistent parse_backslash errors + hinting questions word_parse.py: parse_backslash (errors + hinting questions) Feb 24, 2024
@andychu
Copy link
Contributor

andychu commented Feb 26, 2024

I think this belongs in a new file doc/error-catalog.md and then we should print URLs in the error messages

We want something like this - https://doc.rust-lang.org/error_codes/error-index.html

This kind of description doesn't scale -- there will always be more context and hints you can add

(and it's very hard to read and edit in the source code)

@bar-g
Copy link
Contributor Author

bar-g commented Feb 27, 2024

(and it's very hard to read and edit in the source code)

Yes it is, I noticed that. I think referencing makes sense, to keep it maintainable.

However, for a helpful interpreter I think at least short errors, info and hints must be compiled into the interpreter, and be printed by default. Mentioning some ID, or better even, also printing more detailed info if some doc package is installed could be a separate good thing.

So what's there?

  • short-error-message (what's wrong), better maintained is source code, or not? ever re-used?
  • short-error-info (e.g. what's allowed, reasoning, comparisions)
  • short-bullet-question hints

andychu pushed a commit that referenced this pull request Feb 27, 2024
Addresses issues in pull request #1824.

Now we have 4 hints!
@andychu
Copy link
Contributor

andychu commented Feb 27, 2024

OK I did something about this:

http://travis-ci.oilshell.org/github-jobs/6335/ovm-tarball.wwz/_release/VERSION/doc/error-catalog.html#oils-err-10

36fcf3d

You can download a new tarball from the CI if you want to check it out (task cpp-tarball or ovm-tarball, let me know if you haven't done that before)

Feedback welcome!

Instructions on how to update it are at the top of the doc. That is welcome too

Although really we need to link more to the Oils reference, which is ref/chap-*.html etc. I might be missing a couple links there

And then we need to write missing help topics too

@bar-g
Copy link
Contributor Author

bar-g commented Feb 28, 2024

Ok, let's see how that goes.

But don't you think adding at least one zero to each error category would make sense?

100->1000
200->2000

@bar-g
Copy link
Contributor Author

bar-g commented Feb 28, 2024

I looked at the examples that you provided, and saw you only added some error explaining info after the bullet-question hints. (Wereas I had been adding such short error info (context/reason/what's good) right behind the error (what's wrong).

May we add the error info lines before the question-hints, as ordered list maybe?

And quite important, would you actually accept the lines that I prepared in the pull requests so far?

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.

2 participants