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

Line numbers for better error handling (WIP) #42

Merged
merged 4 commits into from
Feb 27, 2018

Conversation

kunal-mohta
Copy link
Contributor

This presents a proposal for solving #40.
I have used the method to solve #30. You can check the method by trying to reproduce the mentioned there.
If this looks fine, I can try to implement this on all the errors thrown in the code.

@stebanos
Copy link
Member

I'm not a fan of having the line number in Lexer. Since Parser manages the progress of the Lexer, it could catch the errors thrown in Lexer (and itself) and rethrow them with the line number added. That way, errors in Lexer don't need to be rewritten. Not for that reason anyway. Also for added consistency with the code we have already the error message should start with Line ${lineno}:

@kunal-mohta
Copy link
Contributor Author

@stebanos This looks better?

@stebanos
Copy link
Member

Hey Kunal, yes this is better. Will you also add in the error re-throwing?

@kunal-mohta
Copy link
Contributor Author

Sure!

@kunal-mohta
Copy link
Contributor Author

There are still errors thrown in the Interpreter class that need line numbers.
I guess that would need line numbers to be present in the phraseBook as well?

@stebanos
Copy link
Member

Hehe, I think you misunderstood my last request. You added Line ${this.lineno}in front of each error. What I meant was something like this:

in the PhraseParser:

    parse() {
        try {
            const node = this.phrase();
            if (this.currentToken.type !== EOF) {
                this.error(EOF);
            }
            return node;
        } catch (e) {
            throw new Error(`Line ${this.lineno}: ${e.message}`);
        }
    }

A similar thing could be done in the Interpreter class if the phrase has access to its line number.

@kunal-mohta
Copy link
Contributor Author

Oh! 🤦‍♂️
I am sorry.. will do that. 😅
For giving Interpreter class the access to line numbers I would need to add line numbers to the phraseBook right? Or is there a better way to do it?

@stebanos
Copy link
Member

The Interpreter can access the phrase through this.phrase, in the screenshot you showed earlier lineno was included in the phrase, so I suppose this.phrase.linenowould just work?

@kunal-mohta
Copy link
Contributor Author

Yes, that is what I plan to do. 👍

@kunal-mohta
Copy link
Contributor Author

Sorry about the commit message 😓

@stebanos stebanos merged commit dc62c22 into nodebox:master Feb 27, 2018
@stebanos
Copy link
Member

Thanks, well done!

@@ -434,7 +434,7 @@ class PhraseLexer extends Lexer {
} else if (this.checkEscapeChar('}')) {
result += '}';
this.advance();
this.advance();
this.advance();line
Copy link
Member

Choose a reason for hiding this comment

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

I know this is now merged, but I don't understand this code. What's the line at the end? Just a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is what it is 😓
I am sorry, let me just change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fdb I have made the correction in #43

Copy link
Member

Choose a reason for hiding this comment

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

👍

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.

3 participants