Add positional information #84
Conversation
tests/Expression.elm
Outdated
| @@ -1,42 +1,34 @@ | |||
| module Expression exposing (all) | |||
| module Expression | |||
There was a problem hiding this comment.
It shouldn't be a part of this PR. It solves different issue
tests/Helpers.elm
Outdated
| @@ -0,0 +1,64 @@ | |||
| module Helpers | |||
tests/Multiline.elm
Outdated
| @@ -0,0 +1,29 @@ | |||
| module Multiline exposing (application) | |||
Attempt to complete location information for expressions
Support for all types has arrived!!! 🎉Huge shoutout to @joeandaverde and @laslowh for their work on making this changes possible. |
src/Ast/Expression.elm
Outdated
| Record | ||
| <$> braces | ||
| (commaSeparated | ||
| ((,) |
There was a problem hiding this comment.
Please try to avoid splitting arguments to next line
Backpipe (<|) twice could do the job here
src/Ast/Expression.elm
Outdated
| (withMeta | ||
| ((\a -> (\m -> ( a, Variable [ a ] m ))) | ||
| <$> loName | ||
| ) |
There was a problem hiding this comment.
Ditto. Could rewritten to something similar to
withMeta <|
Record <$> braces <| comaSeparated <| withMeta <| (\a m -> (a, Variable [a], m) <$> loName)
src/Ast/Expression.elm
Outdated
| \() -> | ||
| chainl | ||
| (withMeta ((\m -> flippedApp m) <$ spacesOrIndentedNewline ops)) | ||
| (term ops) |
src/Ast/Expression.elm
Outdated
| ) | ||
| :: remE | ||
| ) | ||
| remO |
src/Ast/Expression.elm
Outdated
| a | ||
| e | ||
| m | ||
| ) |
tests/Multiline.elm
Outdated
| ) | ||
| (var "k" { line = 12, column = 1 }) | ||
| { line = 12, column = 0 } | ||
| ) |
There was a problem hiding this comment.
Apart from the bug of line numbers (Every argument's line being 1 too small apart from the last one) that I assume is coming from Combine dependency rather then this codebase, the isApplication` could be slightly modified to produce this output
|
Just checking in on this. Has any progress on the line numbering bug been made? |
|
@laslowh So the issue is with lines starting from index 0 (rather than 1) then jumping straight to 2, and treating the last line in the expression as the same as one before. So essentially instead of getting 1,2,3,4,5, we're getting 0,2,3,4,4. No idea what's the cause CC. @baransu |
|
Looks like the line numbering issues were addressed: https://github.com/elm-community/parser-combinators/blob/master/CHANGELOG.md I've started work on fixing the tests and upgrading the package. baransu#2 |
|
Is this PR still active? If not could I help getting this feature merged? |
|
@paulsonnentag there's nobody actively working on this. It'd be great if you tried your shot |
|
@joeandaverde I've checked out your latest commit, but it seems like there are still a lot of cases where the line/column number is not correct. Do you have any tips on what the problem could be? Otherwise, I'll have to dig deeper into the code. |
|
I've started working on this issue this weekend and it looks like it will see a closure soon |
|
@wende If you need any help, let me know 😃 |
|
Closed in favour of #99 |
Huge thanks to @joeandaverde for creating most of what is implemented in this PR in the first place. Also shoutout to the @laslowh who helped with rest of the types. You're the best!!! 🎉
All Expressions and Statements now have Meta as last argument
{ line: Int, column: Int }.Positional information are not perfect. There as some cases where whitespaces are omitted in column count but besides everything works as expected.
This is big breaking change and I'm counting on your feedback.
Fixes: #13
Almost all types are converted contain meta information except:TypeApplicationApplicationBinOpVariableThey are advanced because ofpartial application in case ofTypeApplicationandApplicationcreating in placeVariableandBinOptypes~I was trying implement Meta for those as well but I've failed lacking knowledge and full understanding of
parser-combinatorsas well as advanced Elm. That why I'm creating this WIP PR, counting on your support.~~