You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Unclear error when defining / using an identifier with spaces #30
@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.
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.
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.
@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(?)
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.
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?
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.
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.
Activity
kunal-mohta commentedon Feb 25, 2018
@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 commentedon Feb 26, 2018
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 commentedon Feb 26, 2018
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 commentedon Feb 26, 2018
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 commentedon Feb 26, 2018
@stebanos Exactly, when I was trying to understand the code in class
DefLexerand 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 commentedon Feb 26, 2018
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 commentedon Feb 26, 2018
I think
this.consume(KEY)is the function where the error is being passed for the first case in the issue.🤔
You are referring to this right??
stebanos commentedon Feb 26, 2018
Yes, and also to this (second case).
kunal-mohta commentedon Feb 26, 2018
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 commentedon Feb 26, 2018
Well no, the code does the right thing.
stebanos commentedon Feb 26, 2018
Well as I said before:
if KEY is followed by KEY I don't think it could mean anything else than that there was whitespace in between.
kunal-mohta commentedon Feb 26, 2018
Ohhh 😲
I see!
It works correctly for both the cases. 👍
stebanos commentedon Feb 26, 2018
It works correctly for both cases. The issue is what to communicate to the user.
kunal-mohta commentedon Feb 26, 2018
The position is available, if you want that to be mentioned.
stebanos commentedon Feb 26, 2018
Yes, I think the position is always handy, but probably not enough.
Writing good error messages is hard :ppppp
kunal-mohta commentedon Feb 26, 2018
😅
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 commentedon Feb 26, 2018
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 commentedon Feb 26, 2018
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 commentedon Feb 26, 2018
This deserves its own issue: #40
kunal-mohta commentedon Feb 26, 2018
Was thinking the same! Error handling needs quite a bit of attention.
kunal-mohta commentedon Feb 28, 2018
I guess this can be closed now?
fdb commentedon Feb 28, 2018
@kunal-mohta Please read my note at https://github.com/nodebox/seed/pull/42/files#diff-d85c7ecdc144ac4aec3ee7a0b24c3f9fR437