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

Add encode function for Package.t #4815

Merged
merged 8 commits into from
Jul 20, 2021
Merged

Conversation

shonfeder
Copy link
Collaborator

Adds an encode function for package data, as a prerequisite of adding an encode function for Dune_project.t,

I'm starting with a small chunk of the encoding, so I can get feedback in case I'm going about this wrong.

I could particularly use advise on where/whether I should test this: It will ultimately get tested via black box tests, but seems the encode function itself could use some testing. But I haven't found clear precedent for testing encode functions in the current tests.

@shonfeder shonfeder force-pushed the 4702/package-encoder branch from 4d48d3b to c12f0de Compare July 17, 2021 22:21
@rgrinberg
Copy link
Member

I could particularly use advise on where/whether I should test this: It will ultimately get tested via black box tests, but seems the encode function itself could use some testing. But I haven't found clear precedent for testing encode functions in the current tests.

I don't think tests are necessary specifically for this function. As you said, the blackbox tests will ultimately pick up the errors.

@shonfeder
Copy link
Collaborator Author

shonfeder commented Jul 20, 2021

Ok. Sounds good. I suspect there may be a subtle error or two, but I'll pick those up shortly. Thanks for the review.

@shonfeder shonfeder merged commit 36c9c71 into ocaml:main Jul 20, 2021
@shonfeder shonfeder deleted the 4702/package-encoder branch July 20, 2021 01:52
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.

2 participants