search

alienscience / imapsrv

An IMAP server written in Go
BSD 3-Clause "New" or "Revised" License
48 stars 9 forks source link

Expands lexer tests for astring with closer attention to ABNF in spec #18

Closed RossJHagan closed 9 years ago

RossJHagan commented 9 years ago

@alienscience As you're writing the lexer I've left this as a pull request for your review instead of just forcing it on you.

Some of the test cases do not pass, although I've left them commented out. These need your eye as I'm not confident I can say they should or shouldn't pass - only that the RFC seems to allow them.

I've also reproduced the entire rule from the spec in the comment as a sort of tree - perhaps unnecessarily but just out of temporary convenience. I'm conscious that this will come out as one long weird string under godocs in the long-term so I can happily remove it.

Still lacks a complete set of test cases (e.g. CTL is not tested)

alienscience commented 9 years ago

This is really cool and much appreciated. I won't have a chance to look at this until the weekend though. Please also feel free to hack the lexer when you want to.

alienscience commented 9 years ago

Sorry it took so long. I will look at the failing tests now.

alienscience commented 9 years ago

I fixed some lexer bugs. The EOF failing tests are still marked as TODO.

I personally feel that EOF is a valid panic because it should not occur during normal operation. The parser should only requests tokens from the lexer when the protocol/grammar expects new input. Do you agree or would it be better as an EOF token?

RossJHagan commented 9 years ago

No probs re: speed. No rush at all!

I think you're right. Lines ~88-93 in lexer_test.go can probably go away then. There's likely a better way in which whitelisting panic types can be approached. That was some of the reasoning for that bit of code. The panicCount might mask a panic that you don't want under the guise of 'yep, this case panicked, move along'. Whether that's a relevant concern here or not I'm not sure.