Conversation
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Mostly this is nice - Bracket.hs does feel a little overblown, but generally it seems much nicer/neater than the last time we had bracketing done earlier, and there are some big improvements in error messages etc. too :).
I'm not quite gonna approve though until you can refactor within such that I understand it ;-) (and maybe improve brackets(Worker) :-) )
| ,"at" | ||
| ,show openFC | ||
| ] | ||
| show (UnexpectedClose b) = unwords ["There is no" |
There was a problem hiding this comment.
This is pretty much an OpenCloseMismatch, without an opening (or where the opening is beginning of file) - consider combining the two, i.e.OpenCloseMismatch (Maybe (FC, BracketType)) BracketType. Or not, the conceptual similarity could just be confusing, up to you...
brat/Brat/Lexer/Bracketed.hs
Outdated
|
|
||
| instance Show BToken where | ||
| show (FlatTok t) = show t | ||
| show (Bracketed _ b ts) = showOpen b ++ show ts ++ showClose b |
There was a problem hiding this comment.
show ts here will insert commas between the elements of ts (a list), right? Shouldn't we also insert commas after the opening bracket and before the closing bracket? And presumably we do something about the comma token itself...
There was a problem hiding this comment.
Or use showTokens ts rather than show ts, and then there'll be no separators added between tokens?
There was a problem hiding this comment.
Ok, so I realize you can't showTokens ts here because that needs a Proxy argument but nonetheless, this inserts a comma between the tokens - is that what you want? I tried changing show ts to concatMap show ts which is similar but without the inserted commas and all tests still pass, suggesting we have no coverage of this so we may not even have been aware of the show-inserted commas???
This is targetted at #68. After some preliminary refactorings to remove `Bwd` in #72, I realized `brackets` and `within` were 90% the same, but resisted my first attempts to combine them ;). Changing the contract on `within` (here renamed `helper` and made local to `brackets`) allowed this to proceed. Note the second commit makes explicit, using `Maybe....NonEmpty` an invariant that in the first commit is just comments and incomplete-pattern-matches; you may prefer the first way though. --------- Co-authored-by: Craig Roy <craig.roy@cambridgequantum.com>
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Looks good to me although please have a think about these 3 comments
brat/Brat/Lexer/Bracketed.hs
Outdated
| import Text.Megaparsec (PosState(..), SourcePos(..), TraversableStream(..), VisualStream(..)) | ||
| import Text.Megaparsec.Pos (mkPos) | ||
|
|
||
| opener :: Tok -> Maybe BracketType |
There was a problem hiding this comment.
I suggest
enum OpenClose = Opening(BracketType) | Closing(BracketType) -- maybe there's a better name
openClose :: Tok -> Maybe OpenClose
Then you can do foo ... | Just (Opening b) <- openClose(blah) = ... and so on, and you get a much better case openClose ... of
Then zap both opener and closer, or if you really want, you can do
opener t | Just(Opening b) <- openClose t = Just(b)
opener _ = Nothing
Could also make OpenClose incorporate the Maybe directly inside it, i.e. Open(BracketType) | Close(BracketType) | Neither
brat/Brat/Lexer/Bracketed.hs
Outdated
| closeTok Square = RSquare | ||
| closeTok Brace = RBrace | ||
|
|
||
| eofErr :: FC -> BracketType -> Error |
There was a problem hiding this comment.
I think I suggested you curry this earlier and now I'm gonna make the reverse suggestion - sorry! But the "unit" of bracket, in openCloseMismatchErr, is the (FC, BracketType) and I think it has to be that way, so we should probably use that here and in unexpectedCloseErr too - this would simplify brackets as you would then be able to let-bind locals of type (FC, BracketType)
There was a problem hiding this comment.
I don't think I'm really getting it, but have uncurried anyway
brat/Brat/Lexer/Bracketed.hs
Outdated
|
|
||
| instance Show BToken where | ||
| show (FlatTok t) = show t | ||
| show (Bracketed _ b ts) = showOpen b ++ show ts ++ showClose b |
There was a problem hiding this comment.
Ok, so I realize you can't showTokens ts here because that needs a Proxy argument but nonetheless, this inserts a comma between the tokens - is that what you want? I tried changing show ts to concatMap show ts which is similar but without the inserted commas and all tests still pass, suggesting we have no coverage of this so we may not even have been aware of the show-inserted commas???
|
And, thanks @croyzor - nice work! :-) |
|
Re: |
2 participants
Revived from #32. I was mistaken to close it.