nodebox / seed

Procedural Content Generator
https://seed.emrg.be/
MIT License
22 stars 8 forks source link

Line numbers for better error handling (WIP) #42

Closed kunal-mohta closed 6 years ago

kunal-mohta commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@stebanos This looks better?

stebanos commented 6 years ago

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

kunal-mohta commented 6 years ago

Sure!

kunal-mohta commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Oh! :man_facepalming: I am sorry.. will do that. :sweat_smile: 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 commented 6 years ago

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 commented 6 years ago

Yes, that is what I plan to do. :+1:

kunal-mohta commented 6 years ago

Sorry about the commit message :sweat:

stebanos commented 6 years ago

Thanks, well done!