-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
Completion config file error handling #355
Conversation
|
||
expect { starting_gitsh }.to raise_exception( | ||
Gitsh::TabCompletion::DSL::ParseError, | ||
/Invalid variable \(\$invalid_variable\) at line 1, column 11 in file #{config_path}/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics/LineLength: Line is too long. [96/80]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are too long, but I can't think of a nice way of breaking them up without reducing readability.
299065e
to
9eb4e3a
Compare
@@ -6,7 +6,7 @@ module TabCompletion | |||
module DSL | |||
def self.load(path, start_state, env) | |||
source = File.read(path) | |||
tokens = Lexer.lex(source) | |||
tokens = Lexer.lex(source, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice.
@@ -72,6 +78,8 @@ def self.parse(tokens, opts = {}) | |||
tokens, | |||
opts.merge(env: Environment.new(opts.fetch(:gitsh_env))), | |||
) | |||
rescue RLTK::NotInLanguage => error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to handle any of the other three exceptions that RLTK can raise?
(Maybe not InternalParserError
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to:
RLTK::BadToken
should only happen if our lexer and parser disagree on what the set of tokens is, and I'd hope that would be caught by an integration test rather than raising at runtime.RLTK::HandledError
should only happen if the parser uses error productions, which currently we don't, not least because the RLTK README warns that they're not well tested, and because I don't fully understand what they're for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent reasons. LGTM!
a89a434
to
8c2b0d2
Compare
Loading the tab completion configuration files at start up, instead of lazily loading them on first use, allows us to report errors immediately instead of crashing when the user first tries to use tab completion.
8c2b0d2
to
5d5a924
Compare
This commit introduces useful error reporting when a tab completion configuration file fails to parse. The error will report which token was unexpected, which file it was in, and exactly where in the file.
The tab completion configuration language supports variables which represent matchers for particular types of arguments, e.g. `$branch` or `$path`. This commit introduces error handling when a tab completion configuration file contains a variable that isn't supported by gitsh. The error message will include the name of the variable, the path to the file where the error occurred, and the exact position in the file.
5d5a924
to
1055384
Compare
This PR adds error reporting for problems with the tab completion. It covers several of the issues raised in #316.
Specifically:
??
).$foo
)