json5format icon indicating copy to clipboard operation
json5format copied to clipboard

Consider using nom or another parser-support library?

Open anp opened this issue 5 years ago • 1 comments

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.

anp avatar Apr 24 '20 15:04 anp

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 avatar Apr 27 '20 22:04 richkadel