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

Toml support #300

Merged
merged 15 commits into from
Mar 28, 2019
Merged

Toml support #300

merged 15 commits into from
Mar 28, 2019

Conversation

zekth
Copy link
Contributor

@zekth zekth commented Mar 23, 2019

As told in #232 here is a draft of the TOML parser.

Added the documentation for the module.
Also the handling of Array Of Tables is not finished yet.

Questions:

  • If a declaration is invalid do we skip it or we throw an Exception? Also an option strict could be added to handle both cases.

Note: @GrosSacASac deepAssign function has been added to util/deep_assign.ts

TOML specs

toml/parser.ts Outdated Show resolved Hide resolved
toml/parser_test.ts Outdated Show resolved Hide resolved
toml/parser.ts Outdated Show resolved Hide resolved
@zekth
Copy link
Contributor Author

zekth commented Mar 24, 2019

Does someone have opinion about questions?

cc @hayd @ry @j-f1 ?

@bartlomieju
Copy link
Member

@zekth re: strict option, AFAIK TOML has no strict/non-strict mode, so IMHO it should throw exception when declaration is invalid.

@ry
Copy link
Member

ry commented Mar 26, 2019

@zekth regarding your question - are there any existing libraries we can examine to copy behavior from?

@zekth
Copy link
Contributor Author

zekth commented Mar 26, 2019

@zekth are there any existing libraries we can examine to copy behavior from?

There are some in the TOML wiki, like:

@ry
Copy link
Member

ry commented Mar 26, 2019

@zekth And how do they act on invalid declaration?

@zekth
Copy link
Contributor Author

zekth commented Mar 26, 2019

@ry directly throws exception. It was just an idea i had.

toml/README.md Outdated Show resolved Hide resolved
toml/parser.ts Show resolved Hide resolved
toml/test/arrays.toml Outdated Show resolved Hide resolved
@ry ry marked this pull request as ready for review March 28, 2019 13:37
@zekth
Copy link
Contributor Author

zekth commented Mar 28, 2019

@ry you put it as ready for review, there is still a remaining type handling left. I'm working on it and on some speed optimisations. Do you want to merge it in this state or wait for the last type to be handled? No problem for me in both cases.

@ry
Copy link
Member

ry commented Mar 28, 2019

@zekth I'm ok with merging now - looks useful as it is - but please complete that in follow up PRs

@zekth
Copy link
Contributor Author

zekth commented Mar 28, 2019

@ry ready to ship. Array of tables is handled now, also updated the doc for the warning. I'll do some optimisation in future PRs to fix the speed and remaining cases.

@ry
Copy link
Member

ry commented Mar 28, 2019

Can you copy https://github.com/denoland/deno/blob/master/Cargo.toml to toml/testdata/deno_cargo.toml and check that this can parse it?

@zekth
Copy link
Contributor Author

zekth commented Mar 28, 2019

@ry Ready to merge.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! This is great to have.
LGTM

@ry ry merged commit fa1664e into denoland:master Mar 28, 2019
@zekth zekth deleted the toml_support branch March 28, 2019 17:00
@zekth zekth mentioned this pull request Mar 29, 2019
@bartlomieju bartlomieju mentioned this pull request Apr 1, 2019
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
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

Successfully merging this pull request may close these issues.

3 participants