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

Exception handling PoC #45

Merged
merged 14 commits into from
Nov 16, 2017
Merged

Exception handling PoC #45

merged 14 commits into from
Nov 16, 2017

Conversation

hadrienk
Copy link

This PR is in preparation for #40. The goal is to improve the exception handling in the java-script project.

I created a new ContextualRuntimeException that contains a reference to the ParserRuleContext we get from ANTLR so that we can give the position of an error to the user. I also added VTLScriptException which extends the ScriptException, mainly because the ScriptException constructors are too restricting.

I adjusted AssignmentVisitor, ExpressionVisitor and NativeFunctionsVisitor to use the new exceptions. Take a look at VTLErrorsTest for an example of what the result is.

@statisticsnorway statisticsnorway deleted a comment Nov 15, 2017
Copy link

@pawbu pawbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two comments, looks good otherwise.

};
lexer.addErrorListener(errorListener);
parser.addErrorListener(errorListener);
public VTLParser.StartContext compile(Reader reader, Consumer<VTLScriptException> errorConsumer) throws IOException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the compile word here. In antlr it is used in the context of compiling a grammar file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, how about validate?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe getRootContext or getStartContext. Or maybe split this method as it sets up lexer and parser and also gets the start context.

), ctx
);
}
// checkArgument(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove if not needed?

@hadrienk hadrienk merged commit a2e455b into develop Nov 16, 2017
@hadrienk hadrienk deleted the feature/exceptions branch November 16, 2017 08:53
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