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

Consider using nom or another parser-support library? #5

Open
anp opened this issue Apr 24, 2020 · 1 comment
Open

Consider using nom or another parser-support library? #5

anp opened this issue Apr 24, 2020 · 1 comment
Labels
enhancement New feature or request long term Deferred lower priority issue

Comments

@anp
Copy link

anp commented Apr 24, 2020

The current regex parsing strategy looks like it works quite well and is very well tested, but I have concerns about the efficiency and maintainability of regex-based parsers. This (really awesome!) tool seems likely to be with us for a long haul so I'd encourage the authors to look into using a crate like nom or pest for handling the actual parsing. nom has a fairly concise example of using it to parse json which I imagine could be adapted to support json5 and the additional parsing done there.

@richkadel richkadel added the enhancement New feature or request label Apr 27, 2020
@richkadel
Copy link
Collaborator

Thanks for the suggestion! I'm sure it's possible to use something like nom or pest to support the parsing, but the reason I didn't start with this approach was twofold:

  • I found there were subtle complexities to preserving certain structure and format characteristics, such as preserving indent within a comment block, and I found it easier to reason about the content by interacting with the token space at a lower level. To do the same with a higher-level parser library can sometimes be at least as hard if not harder.
  • The json5 syntax is fairly trivial and I have extensive experience using Regex. I am not too concerned about the maintainability here. I'm not sure if there will ever be a need to make changes to this grammar. (Never say never, though.)

A change like this is pretty major, and I don't think you are suggesting the result will provide any new capability. Given the current implementation works (and is well tested as you point out), I think it's safer to leave this as-is.

Let me know if you have other thoughts.

Thanks for reviewing!

@richkadel richkadel added the long term Deferred lower priority issue label Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request long term Deferred lower priority issue
Projects
None yet
Development

No branches or pull requests

2 participants