Skip to content
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

Comments make parsing fail #41

Open
joonazan opened this issue May 18, 2017 · 15 comments
Open

Comments make parsing fail #41

joonazan opened this issue May 18, 2017 · 15 comments
Labels

Comments

@joonazan
Copy link
Contributor

f =
  let b = 1 --comment
  in b

The parser does not understand this.

This is probably related to #1, because this, too has to do with newlines.

@wende
Copy link
Collaborator

wende commented May 18, 2017

Yes. Comments right now are only statements (outside of functions). We're working on a way to go past that, but it's not trivial

@dynajoe
Copy link

dynajoe commented May 18, 2017

It may be helpful to do multiple passes of the source. One of which might be filtering out comments.

@wende
Copy link
Collaborator

wende commented May 18, 2017

Yeah. The parser doesn't really have a capability for us to do it.
In elmchemy right now we're removing inline comments before we parse the code.

@wende wende added the wontfix label May 18, 2017
@joonazan
Copy link
Contributor Author

I think it would make sense to move the code from Elmchemy here. I don't see why someone would want the parser to fail instead of stripping comments.

@wende
Copy link
Collaborator

wende commented May 23, 2017

@joonazan But we aren't really parsing them, we just ignore them. So that's probably not a desirable behaviour for a parser 😁

@justuswilhelm
Copy link

It would be awesome if this can be resolved. Elm-Ast is already providing a lot of value.

Another minimal example that causes a failure:

test : Int
test =
    --
    1

@YetAnotherMinion
Copy link

YetAnotherMinion commented Oct 31, 2017

I did not see a lex combinator defined or used in this codebase. When I look at parsers for other languages I frequently see a lex combinator used to remove the whitespace and comments. Here is an example of parsing JSON using a combinator library in rust https://github.com/Marwes/combine/blob/master/benches/json.rs.

Something like:

lex : Parser s r -> Parser s r
lex p =
    p <* (skipMany (whitespace <|> comments))

Then just wrap just about every low level parser with lex.

@wende wende changed the title Comments ruin everything Comments make parsing fail Dec 10, 2017
@dillonkearns
Copy link

But we aren't really parsing them, we just ignore them. So that's probably not a desirable behaviour for a parser 😁

@wende to your point, since this is not parsing these statements but throwing them away, what if we had an alternative function that acknowledged this? Something like Ast.discardCommentsAndParse? It could do a first pass to throw out comments, and then do the parse to ensure comments don't cause parsing errors. This issue seems to be causing some errors in people's code so it would be nice to offer a workaround.

@wende
Copy link
Collaborator

wende commented Jan 12, 2018

@dillonkearns I like the idea to make it configurable. Could fit the next major version 👍

@dillonkearns
Copy link

@wende that would be outstanding! It would really help me unblock some of my users in https://github.com/dillonkearns/elm-typescript-interop.

@dillonkearns
Copy link

Hey @wende any update on this? I still have some users who are blocked on this, it would be great to be able to have some workaround to help strip comments before parsing.

@Birowsky
Copy link

Pinga wanga

@muelli
Copy link

muelli commented May 15, 2018

FTR: @tunguski has a fork here: https://github.com/tunguski/elm-ast that seems work well with comments. At least the example page https://tunguski.github.io/elm-ast/example/ parses the code from #41 (comment)

tunguski#3 refers to tunguski@60a771c

@dillonkearns
Copy link

Hello @wende, it looks like comments are parsing successfully now! Thank you so much for adding that in. Both -- ... and {- ... -} comments are parsing as expected, so this looks to be fully resolved now.

Would you mind closing the issue so it's clear for other users who were waiting for this? Thank you!

@dillonkearns
Copy link

I apologize for the confusion, the examples I tried for parsing comments were not within function bodies (like in this example). So it turns out the behavior hasn't changed and comments like these still cause parsing to fail.

I did indeed find that https://github.com/tunguski/elm-ast resolves the issue, as @muelli suggested (thanks for that tip @muelli!).

I would still love to see this get resolved in this package! It would be nice if we could avoid having too much fragmentation in the Elm AST library ecosystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants