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

Change the way of toml parsing. #17

Closed
serayuzgur opened this issue Jul 15, 2018 · 6 comments
Closed

Change the way of toml parsing. #17

serayuzgur opened this issue Jul 15, 2018 · 6 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@serayuzgur
Copy link
Owner

Due to limitations of the toml parser code has lots of regex and performance may be damaged by that.

@serayuzgur serayuzgur added enhancement New feature or request help wanted Extra attention is needed labels Jul 15, 2018
@serayuzgur serayuzgur self-assigned this Jul 15, 2018
@serayuzgur
Copy link
Owner Author

serayuzgur commented Jul 20, 2018

Hi @userzimmermann ,
Since you sent regexp PR's, I think this test will be secure at you hands. Can you help me to close this issue ? If yes, please pull master and test a build from that.

I wrote the parser from stratch. All alternatives were bad or I was unable to make them work. I know it is risky but you know it is only version indexing.

@zimmermannmobile
Copy link

Nice work, @serayuzgur! Of course I'll help

Which alternatives did you try? I would also like to take a look at them. And we could surely also simplify your parser with some regexps... :)

@serayuzgur
Copy link
Owner Author

Thanks for the help,I tried the projects listed below.
BinaryMuse/toml-node
jakwings/toml-j0.4
JonAbrams/tomljs
alexbeletsky/toml-js
ldthomas/apg-js

Yes we can simplify it :) actually it is draft work and open for all kind of development.

@serayuzgur
Copy link
Owner Author

Hi @zimmermannmobile
What do you think? Did you find any usable solution?

@userzimmermann
Copy link
Contributor

Hi @serayuzgur

With also accepting \r as whitespace in your custom TOML parser (PR #21), the whole extension works fine for me so far in the current state

It's also nice that your re-design implicitly makes same dependencies in multiple [target...] tables getting treated separately regarding to version check, which didn't work before. I originally wanted to raise an issue about that, but then had somehow forgot about doing so :D

I was able to create a JavaScript parser grammar with https://github.com/ldthomas/apg-js2 from https://github.com/toml-lang/toml/blob/master/toml.abnf but didn't figure out so far how to create the complete parser from all the apg components. I'm thinking about creating a separate project for a generic TOML parser libraray, which I can then also re-use for a VSCode Python dependencies extension

@serayuzgur
Copy link
Owner Author

@userzimmermann Thanks for all of your work. I will megre PR and you name will be at both change log and readme
For apg-js2 good luck :)

serayuzgur added a commit that referenced this issue Jul 28, 2018
* RegExps removed and migrated to a custom parser.  [#17](#17).
* Additional depencency case added for the test. [#22](#22).
* All failed fetchs will be reported via status mesage and notification with a *retry* command [#23](#23).
* A bug at caching fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants