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 autoformatting to CI (for Markdown, YAML, JSON) #95

Merged
merged 18 commits into from
Oct 8, 2023
Merged

Add autoformatting to CI (for Markdown, YAML, JSON) #95

merged 18 commits into from
Oct 8, 2023

Conversation

fredrik-bakke
Copy link
Contributor

@fredrik-bakke fredrik-bakke commented Oct 6, 2023

If I didn't mess this up, it will autoformat markdown (and json and yaml) files according to our settings in .prettierrc.json on commits to the main branch, i.e. when merging pull requests.

@fredrik-bakke
Copy link
Contributor Author

fredrik-bakke commented Oct 6, 2023

In the Rzk workflow, we currently explicitly pass the files that are checked:

      - name: Check all files
        uses: rzk-lang/rzk-action@v1
        with:
          rzk-version: latest
          # rzk-version: v0.4.1.1   # example of a specific version
          files: src/hott/* src/simplicial-hott/*

Has this action been updated to be able to recognize that there is an rzk.yml file in the root so we can use that instead, @fizruk?

EDIT: I made a pull request: rzk-lang/rzk-action#3 :)

@fredrik-bakke fredrik-bakke changed the title Add formatting check to CI Add autoformatting to CI Oct 6, 2023
@fizruk fizruk changed the title Add autoformatting to CI Add autoformatting to CI (for Markdown, YAML, JSON) Oct 6, 2023
@fizruk
Copy link
Member

fizruk commented Oct 6, 2023

I've updated the title of the PR, since I originally thought this will autoformat Rzk code somehow.

@fredrik-bakke
Copy link
Contributor Author

Thanks! I wish...

@fizruk
Copy link
Member

fizruk commented Oct 6, 2023

Thanks! I wish...

I think it should not be too difficult to implement most of the styleguide in a pretty-printer/autoformatter in rzk itself, and making it available via the VS Code extension. I think even some checks for the naming conventions can be automated. We'll look into that at some point, I think. But probably not in October :)

@fizruk
Copy link
Member

fizruk commented Oct 6, 2023

I'm not sure how autoformatting after this is merged will affect current PRs. Perhaps, we should merge #93 first, then this.

@fredrik-bakke
Copy link
Contributor Author

There's no rush, but it should minimally affect PRs. The workflow only activates on commits to the main branch, i.e. after the PR is merged. It is also configured only to autoformat files touched by the relevant PR.

@fizruk
Copy link
Member

fizruk commented Oct 6, 2023

The workflow only activates on commits to the main branch, i.e. after the PR is merged.

Isn't it activated on the merge commit that happens when we merge this PR?

@fredrik-bakke
Copy link
Contributor Author

I assume it does, but this PR doesn't touch any of the files that the other PR touches.

@fredrik-bakke
Copy link
Contributor Author

This is of course assuming the workflow is set up properly.

@emilyriehl
Copy link
Collaborator

I don't entirely understand what is going on here so I'll leave it for you to decide :)

@fredrik-bakke fredrik-bakke merged commit 565122b into rzk-lang:main Oct 8, 2023
@fredrik-bakke fredrik-bakke deleted the ci-formatting branch October 8, 2023 14:50
@fredrik-bakke
Copy link
Contributor Author

This is of course assuming the workflow is set up properly.

Turns out I didn't 🙃

@fizruk
Copy link
Member

fizruk commented Oct 8, 2023

@fredrik-bakke you can create a separate branch and test it out in it. You should have Write access to create new branches in this repo directly, instead of using a fork.

@fredrik-bakke
Copy link
Contributor Author

Ah, thanks! Yep, this wasn't the best approach

@fredrik-bakke
Copy link
Contributor Author

Turns out the workflow needs a github token with write access, but I don't think I have the privilege to generate those

@fizruk
Copy link
Member

fizruk commented Oct 8, 2023

@fredrik-bakke you should be able to just add something like the following to the job in the workflow file:

  permissions:
    contents: write

For example, see https://github.com/rzk-lang/rzk/blob/530666653a308a131da9225f7f9426d1b0a65a3b/.github/workflows/mkdocs.yml#L13-L14

@fredrik-bakke
Copy link
Contributor Author

Ah, wow, thanks! That seems to have fixed it. I made a new pull request #96.

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