search

ocaml-toml / To.ml

OCaml library for TOML
https://ocaml-toml.github.io/To.ml
Other
82 stars 20 forks source link

Array of tables #16

Closed Emm closed 9 years ago

Emm commented 9 years ago

The Christmas pull request. Array of tables for the people.

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling 0b80d9978bf5470d0a917a079c2d1210a46ce585 on Emm:array_of_tables into \ on mackwic:master**.

sagotch commented 9 years ago

:+1: :+1: Really good news! :+1: :+1:

Some comments about coding style.

I did not deeply checked the code, but it looks good to me. Nothing seems broken, which is the most important anyway :-)

Thank you for this.

mackwic commented 9 years ago

:santa: That's the chrismas spirit ! :gift: under the :christmas_tree: :)

Code looks good :+1: LGTM, and thank you for the PR.

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling 42ea9f95f9edafd4583307ca21ef321abad6b906 on Emm:array_of_tables into \ on mackwic:master**.

Emm commented 9 years ago

While we're at it... What should we do with the Dumper module? Currently, it's useless, because it operates on internal types, but we use the abstract types in our tests. I think we should keep doing that (it's a good way to detect design issues in the API), but it also means we don't have a pretty-printer for our tests, and it makes debugging more painful than necessary. That said, we could expose it, which would give us back our pretty-printer, and it may be useful for end-users as well. It does break encapsulation, but no sane user would rely on parsing strings returned by a pretty-printer.

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling c9bcb0a8fee6c37af19162ebbb469f2a44959ffc on Emm:array_of_tables into \ on mackwic:master**.