Skip to content
This repository has been archived by the owner on Jul 25, 2024. It is now read-only.

Support line numbers in parser #32

Open
joshprice opened this issue Dec 24, 2015 · 9 comments
Open

Support line numbers in parser #32

joshprice opened this issue Dec 24, 2015 · 9 comments

Comments

@joshprice
Copy link
Member

Also column numbers if possible if leex supports this

@joshprice
Copy link
Member Author

Would like to integrate https://github.com/asonge/graphql at some point as a configurable parser option, this will allow better error messages in the absence of being able to make leex and yecc do this.

@joshprice
Copy link
Member Author

Integrating with https://github.com/aarvay/graphql_parser is also an option but less desirable in production because of the ability of NIF to crash entire nodes

@bruce
Copy link
Contributor

bruce commented Feb 16, 2016

Some notes:

  • Leex doesn't support column numbers yet (you probably already know this); I've gone through the source to verify, but haven't looked at the feasibility of adding it.
  • Absinthe's modifications added support for line numbers in Dec (mostly by stopping name tokens discarding the line number info): https://github.com/CargoSense/absinthe/blob/master/src/absinthe_parser.yrl
  • Alex' parser seems like a good option; I've pondered doing that as well, but on the backlog, and would be a full switch after benchmarking. No plans for a configurable parser on my side.

@joshprice
Copy link
Member Author

I don't think I recorded it anywhere but @rvirding made a comment in a talk some time ago about fixing leex to record column numbers. Like you I hunted for it but found no trace of this actually happening. Last time I looked nothing had happened on leex in GitHub for quite some time.

@joshprice
Copy link
Member Author

Not sure what you mean by Dec?

Suspect a configurable parser would be a transient option in order to perform benchmarking and other comparisons (error msgs, etc). Once this was done, a single parser should emerge victorious from the battle.

That said, they all have rather different features, so I'd probably leave a fairly trivial config in place even if just to track the reference parser.

To summarise the pros and cons of each:

  • leex/yecc is erlang standard, easy to extend and reasonably snappy but lacks ending line numbers and completely loses column info. Also relatively poor error messages. Not sure how easy it is to do anything about these problems.
  • @asonge's parser is probably the best overall approach in terms of speed and great error messages but loses points on being custom and harder to understand/extend on that basis. Would love to see where this can go though!
  • libgraphqlparser is the reference impl in C++ and therefore correct and fast. However with @aarvay's NIF wrapper has less than desirable production characteristics. I think it worth tracking for comparison purposes (ie proving compatibility, performance baseline) longer term but wouldn't want to put it live in the same VM as core GraphQL since it could crash and take the whole thing down. There maybe ways around this though.

@bruce
Copy link
Contributor

bruce commented Mar 2, 2016

@joshprice I meant "in December." I agree on @asonge's parser, as well. I think it probably has the best featureset, but fast custom parsers are always harder to read than configuration for parser generators.

@rvirding
Copy link

rvirding commented Mar 2, 2016

Re leex, no nothing has happened with it for a while, mainly due to lack of time, not lack of interest. I will fix it when I get the time.

@joshprice
Copy link
Member Author

@rvirding thanks so much for the update, is there any context you might be able to provide so that someone else might be able to look into it?

@joshprice
Copy link
Member Author

I've opened rvirding/leex#3 to track this

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

No branches or pull requests

3 participants