add a CI to format the code

[atxr]

Add github workflow to format code when pushing on main

Fixes #82

[atxr]

Sorry for the force push, it was to resolve conflicts

[atxr]

I understand I modified many things but if you want to take a close look you can:

• check the prettier config files (I think there are the same for backend and frontend)
• try to understand the github action and see if we agree on it (regarding the opened discussion, there is still some work here)

[amtoine]

i think the CI looks good. but why doesn't it run? :sweat_smile:

[atxr]

I think it doesn't run on PR's for now, we need to push it first

[amtoine]

I think it doesn't run on PR's for now, we need to push it first

that is strange. how do you test a CI before merging? it could be very broken and block all the new PRs on top of main :thinking:

and i remember CIs running as soon as they have a .github/workflows/*.yml file on top of the PR branch :thinking:

[atxr]

Ok! Here is the UPDATE The CI doesn't run because I'm working on a branch in my fork I tested firstly on a dummy repo that I deleted, and now, I opened a dummy PR on my fork to test the CI, and it works! :partying_face:

So as soon as it will be merged here, it will start don't worry @amtoine ! I also solved all the discussion above with the last test, but I'll let @ctmbl close them

Here is the error message that we will get when the CI fails: You can also check the test PR BUT PLEASE DON'T MERGE IT (it's a draft, so it shouldn't happen) :smiling_face_with_tear:

Finally, I did a test with a none formatted file to see if the CI fails, and it worked. I add some trouble then when reverting the commit (because I was working on the sh*tty vscode online version that auto-format on save with a different prettier config) but now everything is ready to be merged.

[ctmbl]

I think someone should mention that this action comes from https://github.com/creyD/prettier_action

[ctmbl]

we cannot have a top-level prettier.config.yml file, instead of two file in backend/ and frontend/? or maybe you plan to apply multiple different rules?

https://github.com/iScsc/iscsc.fr/pull/83#pullrequestreview-1365565616 @amtoine @atxr this hasn't not been answered if I'm not mistaken

[ctmbl]

but why doesn't it run? :sweat_smile:

As @atxr discovered and epxlained it after your question @amtoine, it will run once we click the Squash and merge button, because the action states:

on:
  pull_request:
  push:
    branches:
      - main

it doesn't trigger on pull request, only on push (THAT'S WRONG see my other comment), a merging is a push (ordered by GitHub) so it'll trigger when we'll decide to merge. This is why I wrote this in a thread above.

Why I find this awful

Imagine a workflow:

• I create a PR
• you review it
• I commit modifications
• You approve
• I merge it (finally) This is a quite long process already, even if I truly believe that it is a good and necessary one.

However after this PR the journey won't be over, we'll add a (not necessary in my opinion) overhead: because we trigger the action only on push teh CI will fails when trying to merge, rejecting the merge, requiring a new commit, dismissing the approvals of the reviewers (becuase the Dismiss stale pull request approvals when new commits are pushed option is ON on this repo's main protection rule see) and don't get me wrong this is a good policy I want to keep it)... Forcing the reviewers to re-approve, formatting... awful...

SO, I think we should instead trigger the CI on pull_request and push, the PR author will be warned to remember the formatting long before the merging, and anyway the merging will be blocked if the formatting isn't done :+1:

on:
  pull_request:
    branches:
      - main
  push:
    branches:
      - main

looking forward for your POV!

EDIT: I was arguing in the void this was only a misunderstanding! see my comment and @amtoine 's comment below

[amtoine]

@ctmbl i thought

pull_request:

meant "any pull request" :open_mouth:

not sure it's very explicit in the doc here :thinking:

and again, see the on field of the ci.yml of nushell, it does not define anything in on.pull_request yet the CI runs on every single PR :confused:

i'm actually genuinly confused by this...

[ctmbl]

@amtoine

meant "any pull request" 😮

And actually you're right!! (and I was wrong) (and we agree on the trigger events we want, that's truly beautiful) The only reason why the workflow didn't trigger here is thta the file, at the moment (BEFORE the merging) only exists on atxr/iscsc.fr then can't trigger a workflow here 🥲 @atxr stated it here:

The CI doesn't run because I'm working on a branch in my fork

but I didn't get it exactly when reading it, now I do!

[atxr]

@ctmbl @amtoine conflict resolved! :heavy_check_mark:

[atxr]

Yes sorry I didn't know how to do without a force push that was weird 🙄

[amtoine]

Yes sorry I didn't know how to do without a force push that was weird roll_eyes

i think the easiest for authors and the less confusing for reviewers is to

• merge the current conflicting main branch, even better the exact revision
• add in the commit message that it's a merge to solve conflicts

that would

• avoid force-pushing
• leave a "linear" history for the reviewers to checkout the new branch without too much intervention
• avoid making all the commits of the PR potentially bad
  • with a merge, you only have to review the new commit
  • with a rebase / force-push, i do not have any guarantee that you did not break anything during the rebase :thinking:

[amtoine]

but thanks a lot for the contribution, that's really great :relieved:

looking forward to seeing the CI catch formatting mistakes :smirk:

[ctmbl]

@atxr

I completely agree with @amtoine about merging to solve conflicts due to other PR merges that happened during the reviewing process!

[atxr]

Alright, I was always using rebase indeed, but a merge is more appropriate for these cases.

[amtoine]

no worries @atxr

git rebase is great but i learned to dislike it when done for no strong reason in the middle of a review :wink: