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

Proof of concept of CB-checker #416

Closed
wants to merge 10 commits into from
Closed

Proof of concept of CB-checker #416

wants to merge 10 commits into from

Conversation

ElectreAAS
Copy link
Contributor

@ElectreAAS ElectreAAS commented Feb 27, 2023

This PR creates two new packages: cb-schema which is used internally as the one-stop API, and cb-check which is an executable taking in a file or stdin and checking that the produced json is of the correct format.

This would probably solve #385.

PS: I unified the various .ocamlformat files that were lying around for some reason, sorry for the noise

@ElectreAAS
Copy link
Contributor Author

This is in draft format for the moment, because I can't get the packaging right, but the main code is ready for review @art-w @punchagan !

@ElectreAAS ElectreAAS requested review from art-w and punchagan and removed request for art-w February 27, 2023 16:29
cb-check/cb_check.ml Outdated Show resolved Hide resolved
| None -> ()
| Some l ->
let merged = Cb_schema.S.merge [] l |> List.map Cb_schema.S.to_json in
Format.printf "Correctly parsed %d benchmark(s):\n" (List.length merged);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: we may have parsed more valid jsons than what remains after the merge (which could collapse everything down to one result :P )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that in some cases, even though a merge would succeed, it would not be desirable? Or that we need to treat the result of the merge as a list?

Copy link
Contributor

@art-w art-w Mar 1, 2023

Choose a reason for hiding this comment

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

Nah nothing that complex, my comment was just about the phrasing of "**Correctly parsed** %d benchmarks" (the list validated holds the number of correctly parsed jsons, while the list merged can be much smaller after merging all the jsons together)

(I don't know that we care about either length, so perhaps we can just drop this debug output ^^)

merged

let () =
let ic = if Array.length Sys.argv >= 2 then open_in Sys.argv.(1) else stdin in
Copy link
Contributor

Choose a reason for hiding this comment

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

As a later improvement, it might be worth looking into Cmdliner (only for its ability to generate a user friendly --help man page, we don't need crazy command line arguments... for now :D)

@ElectreAAS ElectreAAS force-pushed the cb-check branch 2 times, most recently from 30e6cee to 60f8577 Compare February 28, 2023 16:44
@ElectreAAS
Copy link
Contributor Author

The new commit history should be much clearer, and allow for more precise reviewing.
In the current state, the project doesn't build due to, I assume, linking problems at runtime (cb-schema not being available in dockers probably)

@ElectreAAS ElectreAAS force-pushed the cb-check branch 2 times, most recently from 5f4d255 to 658b1b6 Compare March 3, 2023 19:17
@ElectreAAS
Copy link
Contributor Author

As #422 is now merged, the content is upstreamed, and with #424 the noise of the packaging has been resolved. I'll close this PR and associated issues as completed.

@ElectreAAS ElectreAAS closed this Mar 24, 2023
@ElectreAAS ElectreAAS deleted the cb-check branch March 24, 2023 15:17
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.

None yet

2 participants