serde-ml / serde

Serialization framework for OCaml
MIT License
174 stars 11 forks source link

Update tests to match new implementation #36

Open maxRN opened 1 month ago

maxRN commented 1 month ago

Hi! I had a look at the test failures and they seemed to be mostly just formatting differences and nothing implementation related so I went ahead and promoted the changes.

Two stylistic changes were responsible for the bulk of the changes:

One strange thing: the tests were throwing errors that [@@@warning "-37"] wasn't allowed to appear at that location. I'm not sure this is true, but removing it resolved those errors and didn't introduce any new errors. There were some regressions in the compiler regarding top-level placement of attributes: https://github.com/ocaml/ocaml/pull/13170 but I couldn't tell if they were already implemented in 5.2. In any case, should those errors re-appear in a new release we can just add back those attributes. (Or maybe put it in a dune file? See https://stackoverflow.com/a/57120928/16727853)

I'm not familiar with serde's codebase so I hope I didn't miss anything, but I think most changes were pretty straightforward.

maxRN commented 1 month ago

Sorry, having some serious git skill issues right now. Will push a new version soon.

maxRN commented 1 month ago

Now it's fixed and rebased on the latest branch @leostera