Unclear error when defining / using an identifier with spaces

[fdb]

Phrase identifiers can't have spaces in them. If users accidentally do add them, the error is unclear:

This is the error when defining a block with an invalid identifier:

---

---

This is the error when using an invalid identifier:

---

[kunal-mohta]

@fdb This is more difficult than it seems to be. It is very difficult to trace back and find out what is the exact point where the cause of the error lies, and I think that for the first case it lies at the nextToken() function here. I might be wrong though! Even after finding the cause, I find it difficult to fix this without destroying the existing well-structured code.

[fdb]

Alternatively, instead of doing this during the compilation pass, it could also be done in a validation pass, that quickly goes over the document and checks for lines that start with a character and end with a :. Those lines should never have spaces in them.

[kunal-mohta]

Ohkay.. So we can write the check here..? Also, should there be no space at all in these lines? Like should root<space>: be invalid too?

[stebanos]

The checks already happen at the compilation stage. I think the issue is that the error messages produced by DefParser (and also those by PhraseParser) should be more evolved than they are now. Personally I don't think root<space>: should be invalid, whitespace is discarded regardless.

[kunal-mohta]

@stebanos Exactly, when I was trying to understand the code in class DefLexer and it sub-classes, I realized that the check is already there, but there are no errors thrown specifically for these cases, and so they end up with the errors we are getting now. I think that attempting to make changes in these classes to get the correct error message for these cases might lead to the disruption of the currently well-built flow of control. It is already very difficult for me (as a newbie to the code) to identify the exact flow of control. So @fdb's method might be more efficient(?)

[stebanos]

I think all this needs after where in the source: this.consume(KEY);

we check whether if (this.currentToken.type === KEY) { throw new Error('Whitespace in identifiers not allowed.'); } Well something more elaborate than that, possibly with the line number and what string has been encountered.

This will probably suffice because logically a key token cannot be followed by another key token.

[kunal-mohta]

I think this.consume(KEY) is the function where the error is being passed for the first case in the issue. :thinking: You are referring to this right??

[stebanos]

Yes, and also to this (second case).

[kunal-mohta]

Okay, but the error is cause inside the consume() function (I think). So the execution is being stopped there, how can we check the condition you are suggesting after the error has been thrown?

[stebanos]

Well no, the code does the right thing.

1. parser encounters KEY
2. this.consume(KEY) -> no error
3. parser encounters another KEY, but the parser logic doesn't allow this
4. this.consume(currentToken whatever it may be) -> error that we see now.

[stebanos]

Well as I said before:

this.consume(KEY); // -> currentToken is now something else but should not be KEY
if (this.currentToken.type === KEY) { throw new Error('Whitespace in identifiers not allowed.'); }

if KEY is followed by KEY I don't think it could mean anything else than that there was whitespace in between.

[kunal-mohta]

Ohhh :astonished: I see! It works correctly for both the cases. :+1:

[stebanos]

It works correctly for both cases. The issue is what to communicate to the user.

[kunal-mohta]

The position is available, if you want that to be mentioned.

[stebanos]

Yes, I think the position is always handy, but probably not enough. Writing good error messages is hard :ppppp

[kunal-mohta]

:sweat_smile: Well I was wondering if it is possible to get line numbers here too. But I guess that it would involve rewriting some of the previous code?

[stebanos]

Having the line numbers available at the parsing/runtime stage would definitely be great! It would indeed involve some rewriting. Also, in theory it's possible that a tag might end on a different line than where it began, in that case it becomes even more complex.

[kunal-mohta]

Right. Perhaps there needs to be a function that keeps track of the line numbers and is called upon every time when we need the updated line numbers?

[stebanos]

This deserves its own issue: #40

[kunal-mohta]

Was thinking the same! Error handling needs quite a bit of attention.

[kunal-mohta]

I guess this can be closed now?

[fdb]

@kunal-mohta Please read my note at https://github.com/nodebox/seed/pull/42/files#diff-d85c7ecdc144ac4aec3ee7a0b24c3f9fR437