-
Notifications
You must be signed in to change notification settings - Fork 564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check warnings/errors for positions #3211
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great idea 👍
|
Look again 😉 maybe GH being slow, it was in the original commit but I amended it and re-pushed before opening the PR. |
fcf9f81
to
5703b4c
Compare
@@ -47,7 +47,6 @@ opTable | |||
-> [[P.Operator (Chain a) () Identity a]] | |||
opTable ops fromOp reapply = | |||
map (map (\(name, a) -> P.Infix (P.try (matchOp fromOp name) >> return (reapply name)) (toAssoc a))) ops | |||
++ [[ P.Infix (P.try (parseOp fromOp >>= \ident -> return (reapply ident))) P.AssocLeft ]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the change from #3212 accidentally sneak in here? ;) https://github.com/purescript/purescript/pull/3212/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't sneak in no, it's kinda required since reapply
needs a source position now, and this case can't provide one (unless / until the entire AST has source positions).
I've been considering how to test it since seeing Phil's reply to #3212, as it didn't break any of the existing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, er, maybe it snuck into this specific PR after all, but one of the others I opened about the same time makes the above comment true.
I'm going to merge this when it actually passes, so when #3208 is finished basically :) |
5703b4c
to
5a7a49b
Compare
This now passes 🎉 (it's based on top of #3318) |
5a7a49b
to
2d5b418
Compare
No description provided.