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

Would you help to compare my parser? #12

Closed
LongTengDao opened this issue Jan 17, 2019 · 6 comments
Closed

Would you help to compare my parser? #12

LongTengDao opened this issue Jan 17, 2019 · 6 comments

Comments

@LongTengDao
Copy link
Contributor

LongTengDao commented Jan 17, 2019

Hello! Would you mind to add my parser to the compare table, after @iarna/toml@1.6.0, toml-j0.4@1.1.1, toml@2.3.3, @sgarciac/bombadil@2.0.0? My parser is https://GitHub.com/LongTengDao/j-toml/, it was published at https://www.npmjs.com/package/@ltd/j-toml.

@iarna
Copy link
Owner

iarna commented Jan 18, 2019

I'll give it a go!

@iarna
Copy link
Owner

iarna commented Jan 18, 2019

@LongTengDao The benchmarks are failing out pretty early with parse errors, for example:

key3 = -2E-2

Throws SyntaxError: Invalid Local Date instead of parsing that as the value -0.02.

I'll make a branch that includes your parser and you can try them out yourself!

I would encourage you to get your parser passing BurntSushi's tests (https://github.com/BurntSushi/toml-test) and/or mine (https://github.com/iarna/toml-spec-tests/tree/0.5.0) before trying to move into benchmarks.

@iarna
Copy link
Owner

iarna commented Jan 18, 2019

It doesn't have to be passing everything for me to include it, but… there are some tests that it seems to infinite loop on, and I can't run it through its paces till you at least fix those bugs.

Specifically, this benchmark 0B-types-scalar-string-multiline-1079-chars just hangs with your parser. That's it trying to parse this file: https://github.com/iarna/iarna-toml/blob/add-ltd-j-toml/benchmark/0B-types-scalar-string-multiline-1079-chars.toml

The branch add-ltd-j-toml adds support to the benchmark suite. It also (temporarily) adds a new file called ltd-benchmark-per-file.js which will run the detailed suite against only your parser, so you can more easily see where the problems are.

@LongTengDao
Copy link
Contributor Author

LongTengDao commented Jan 18, 2019

@iarna Really thanks for your help!

I fixed a data type classification bug. It treated -2E-2 literal with two - as a local date, like 1111-11-11.

I found the infinite-loop-like status comes from a very slow RegExp trap /^(?:[^\\]+|\\[^])*"""/, which should be /^(?:[^\\]|\\[^])*"""/. Just a few characters will make it run a few minutes, amazing.

Then I use the tests to check over my parser (v0.5.42), now it looks good. Wish you run a good result, thank you!

@iarna
Copy link
Owner

iarna commented Feb 18, 2019

@LongTengDao I've updated both the spec compliance test results and the benchmarks with your parser. It still can't complete all the tests, but it's running well enough that I can include it. It's very impressively fast for large strings!

@iarna iarna closed this as completed Feb 18, 2019
@LongTengDao
Copy link
Contributor Author

@iarna Wow! Thank you!

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

No branches or pull requests

2 participants