Fix scanner for Windows line endings and add tests #71
Conversation
| @@ -224,7 +224,11 @@ func (s *Scanner) scanComment(ch rune) { | |||
| // single line comments | |||
| if ch == '#' || (ch == '/' && s.peek() != '*') { | |||
| ch = s.next() | |||
| for ch != '\n' && ch >= 0 && ch != eof { | |||
| for ch != '\n' && ch != '\r' && ch >= 0 && ch != eof { | |||
| if ch == '\r' && s.peek() == '\n' { | |||
There was a problem hiding this comment.
Confused about how if the guard has && ch != '\r' this conditional will ever be true.
There was a problem hiding this comment.
Oh sorry - by "the guard" I mean the one on L227.
There was a problem hiding this comment.
In other words, I believe this will break comments immediately on \r instead of only on \r\n... which might be fine.
There was a problem hiding this comment.
That's a typo (originally the whole guard was one line). Will fix.
There was a problem hiding this comment.
The question is do we want the \r to be included in the comment or treated as part of the line break? Unlike HEREDOCs I'd say they should be part of the line break for comments (though unless we're going to process comments it doesn't make much difference).
|
On first pass something about this looks a little weird. The I didn't check the rest of the code but I believe due to how the scanner works the newline character(s) still show up in the Token text correct? I'm not sure if this is okay or not but it is worth questioning to make sure it is correct. |
jen20
force-pushed
the
b-dos-line-endings
branch
from
3dcff2e
to
665d74b
Compare
|
@mitchellh The loop was a typo - now fixed. The current situation with this PR is:
|
|
@jen20 and I both reviewed pre-rewrite behavior and confirmed that the behavior as he just described it was the same. this LGTM |
Fix scanner for Windows line endings and add tests
I think this should be enough to fix hashicorp/terraform#4069, though this is not yet verified and changes to the parser may also be necessary.