ocaml-community / ISO8601.ml

Parser and printer for date-times in ISO8601
https://ocaml-community.github.io/ISO8601.ml
MIT License
28 stars 13 forks source link

Tests and bug fixes #5

Open dsheets opened 8 years ago

dsheets commented 8 years ago

This patchset introduces a test suite using alcotest which covers parsing, printing, and roundtripping (composed parsing/printing and printing/parsing).

Bug fixes include:

Features include:

sagotch commented 8 years ago

Sounds very nice!

I did not review all the PR deeply, but I need to stop for now.

Just a note about commits 08d88ee84a2fdfd3c5b97af7711532ac6b055ba2, 04ec7d662985a72cb8c6799af90bb8a245974f3f and 8c87fa0fc17313d2af77389c05c4ef55365c7a76: Would it be possible to move test-related commit into the https://github.com/sagotch/ISO8601.ml/tree/test branch which already contains some tests? The way tests are organized is a little bit strange, ISO8601 being separated from tests which use the lib as an external lib to test its API. Also, these test are written with OUnit. I would prefer to keep tests depending on one framework only, so unless there is a good point using alcotest instead, I think OUnit should be kept. Otherwise old tests should be converted to use alcotest as well. Never tried alcotest. so I can not tell, what do you think about it?

And I'll double check changes this evening, but for what I saw it looks like very good to me, really.

sagotch commented 8 years ago

Using Z instead of 00:00 could be optional rather than mandatory. One could want to keep some consistency in printing and do not use the Z form.

What do you think about an optional argument for shortening timezone printing? Also, a character would be needed for custom printing format allowing this, but I see no obvious character to do so...

sagotch commented 7 years ago

Do not lose hope, I will find time to review and adapt this PR...