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

Dune format #21

Merged
merged 1 commit into from
May 31, 2024
Merged

Dune format #21

merged 1 commit into from
May 31, 2024

Conversation

snapdgn
Copy link

@snapdgn snapdgn commented May 30, 2024

Format the json backend with dune fmt.

NOTE: The rest of the sail already uses dune fmt / ocamlformat.
So we can directly use dune fmt to format the whole source code.

  • dune fmt essentially uses ocamlformat under the hood.
  • Formatting on a per file/dir basis can be done with ocamlformat.
    Examples:
    ocamlformat src/sail_json_backend/*.ml --inplace --formats the whole json backend inplace. Use --check to preview the changes first.

@snapdgn
Copy link
Author

snapdgn commented May 30, 2024

I was looking into writing a workflow for Auto formatting the code, but we already have one: https://github.com/ThinkOpenly/sail/blob/json/.github/workflows/formatting.yml, enabling this should do the trick.

@ThinkOpenly
Copy link
Owner

This commit keeps popping up unexpectedly:

[JSON]: Implement support for optional operand

Given the pervasive nature of the changes here, you might need to reset, rebase, rerun the reformat, and force push. Let me know if I can help.

@snapdgn
Copy link
Author

snapdgn commented May 31, 2024

Yeah, I noticed. I forgot to rebase with the upstream. Will cleanup as soon as possible.

@snapdgn
Copy link
Author

snapdgn commented May 31, 2024

@ThinkOpenly , I think this is ready, the formatting workflow ran smoothly (as evident in the checks below), and I've already rebased as well.

Build workflow is failing, I'll look into it and provide a fix through a separate PR.

@ThinkOpenly
Copy link
Owner

Build workflow is failing

It appears to be the same "Test Coverage" action that has failed before this PR, and failing in the same way. I'm asserting that this PR didn't introduce a new build failure, would you agree?

@snapdgn
Copy link
Author

snapdgn commented May 31, 2024

It appears to be the same "Test Coverage" action that has failed before this PR, and failing in the same way.

Yes. But before this both the Test Coverage and Check formatting was failing. Formatting is fixed now.

Yes. This PR didn't introduce a new build failure. It was happening before too.

Copy link
Owner

@ThinkOpenly ThinkOpenly left a comment

Choose a reason for hiding this comment

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

Thanks for discovering this capability, and implementing!

@ThinkOpenly ThinkOpenly merged commit 1974cc5 into ThinkOpenly:json May 31, 2024
1 of 2 checks passed
@snapdgn snapdgn deleted the dune_format branch May 31, 2024 14:49
@joydeep049
Copy link

I was looking into writing a workflow for Auto formatting the code, but we already have one: https://github.com/ThinkOpenly/sail/blob/json/.github/workflows/formatting.yml, enabling this should do the trick.

I just finished creating a Formatter for Ocaml too! But then realised we already have one.
But does our formatter in formatting.yml focus too much on the standards enforced by ocamlformat ??

@ThinkOpenly @snapdgn

@ThinkOpenly
Copy link
Owner

does our formatter in formatting.yml focus too much on the standards enforced by ocamlformat ??

I'm not sure how to answer your question. Are you observing behavior that is concerning?

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.

3 participants