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

Error instead of commit on bad format #97

Merged
merged 9 commits into from
Oct 11, 2023
Merged

Error instead of commit on bad format #97

merged 9 commits into from
Oct 11, 2023

Conversation

fredrik-bakke
Copy link
Contributor

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

Turns out it's not as easy as expected to set up invisible autoformatting. Thus, instead, this PR makes it so the workflow just blocks ill-formatted PRs. Contributors will then have to install an autoformatter (e.g. Prettier) locally to fix the issue.

@fredrik-bakke fredrik-bakke added the bug Something isn't working label Oct 9, 2023
@emilyriehl
Copy link
Collaborator

Hi @fizruk @fredrik-bakke I've lost track of what's up with autoformating. Now whenever I merge something I'm getting an error message about autoformating failure. Can someone explain the current status?

@fredrik-bakke
Copy link
Contributor Author

Hi, the error can be safely ignored. The workflow is just not set up properly.

@fredrik-bakke
Copy link
Contributor Author

This pr makes it so other prs are blocked until they are properly formatted. The recommended procedure should then be for contributors to set up autoformatting in their ide. Given that you agree we should have this feature/rule of course.

@fredrik-bakke
Copy link
Contributor Author

Currently I think it is a good idea to wait until there are few other prs in the pipeline to merge this, so people aren't surprised by it and get weird merge conflicts.

@fizruk
Copy link
Member

fizruk commented Oct 10, 2023

Hi @fizruk @fredrik-bakke I've lost track of what's up with autoformating. Now whenever I merge something I'm getting an error message about autoformating failure. Can someone explain the current status?

  1. Currently we have an autoformatting workflow, but it always fails because it does not have proper permissions to modify main branch`.
  2. The error is benign and can be safely ignored, as @fredrik-bakke mentioned.
  3. This PR changes the behaviour of the workflow: instead of autoformatting and committing to main (modifying the code), the workflow will now only check if formatting is correct and issue (which we can check for incoming PRs and ask contributors to apply autoformatting to the modified files before merging their changes)
  4. If the workflow error (and emails about it) are too bothering, we could disable the workflow (just remove the .github/workflows/format.yml file) until this PR is merged. But I think, this should just be merged soon.
  5. To merge this PR, it is a good idea to merge currently open PRs first (as @fredrik-bakke mentioned, otherwise, contributors might have unpleasant conflicts to resolve locally). Otherwise, I think we could help update PRs that will have conflicts after merging this one.

Note that here we only talk about formatting of config files and text in Markdown (including *.rzk.md), but not Rzk code.

fizruk
fizruk previously approved these changes Oct 11, 2023
@fizruk
Copy link
Member

fizruk commented Oct 11, 2023

@fredrik-bakke can you please update/fix the formatting (I have updated the branch, and it needs some formatting now)?

@fredrik-bakke @emilyriehl I suggest that we merge this before #103 (which supersedes #100) and help @cesarbm03 with his PR in case of conflicts.

@fredrik-bakke
Copy link
Contributor Author

I'll be at my computer in a couple of hours. I can fix it then

This reverts commit d47e9ff.
@fredrik-bakke
Copy link
Contributor Author

Okay, this one should be ready for merging again now. Apologies if this has caused problems for anyone. In #94, I'm adding a remark to the documentation that we enforce autoformatting rules and that contributors should install an autoformatter.

@fizruk fizruk merged commit f88bd3c into rzk-lang:main Oct 11, 2023
@fredrik-bakke fredrik-bakke deleted the ci-format branch October 13, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants