Enforce Prettier formatting?

[fabcor-maxiv]

As noted here: #1312 (comment)

There is a conflict between yamllint and prettier. Somehow this conflict has not appeared in the GitHub checks.

There is no prettier in the pre-commit hooks. But there is prettier in some GitHub Actions workflow.

Should be investigated how come the conflict did not appear. Is the prettier formatting really checked in the GitHub Actions workflow? What is going on?

[marcus-oscarsson]

I think this was the issue:

https://github.com/prettier/prettier-vscode/issues/278

So vscode seems to run the default formater on save if not explicitly told not to for a specific file type. So we need to add
(to .vscode/settings.json):

  "[yaml]": {
    "editor.defaultFormatter": "esbenp.prettier-vscode",
    "editor.formatOnSave": false
  },

EDIT and add yamllint instead

[marcus-oscarsson]

Well, so its more editor configuration issue perhaps but sill worth addressing so that its the same for everyone. I guess I'm not alone to use vscode

[fabcor-maxiv]

I think we should have prettier in the pre-commit hooks, right? At the same time we should remove it from the other GitHub Actions workflow, so that we do prettier formatting in one single place: in the pre-commit hooks. I can probably take care of that.

I do not use vscode, so I will likely not be the one to take care of that part.

[axelboc]

Always good to keep a sanity check on the CI (EDIT: the CI doesn't actually do the formatting; it just checks if it's formatted), as pre-commit hooks are easily bypassed.

[axelboc]

Personally, I prefer format-on-save in the editor instead of commit hooks, as commit hooks can slow down committing and negatively affect DX. But I admit they're convenient in a project like this where not everyone will have their editor configured to format-on-save.

[fabcor-maxiv]

Always good to keep a sanity check on the CI, as pre-commit hooks are easily bypassed.

Personally, I prefer format-on-save in the editor instead of commit hooks, as commit hooks can slow down committing and negatively affect DX.

Yes, sorry, I was being unclear, we already have a GitHub Actions workflow job somewhere that does pre-commit run --all-files:

mxcubeweb/.github/workflows/build_and_test.yml

Line 56 in e193214

run: "${MAMBA_EXE} run --name mxcubeweb poetry run pre-commit run --all-files"

So my plan is:

1. move this out of the workflow it is currently buried in and move it into a more prominent place in its own workflow (right at the beginning so that we do not waste time running any other GitHub checks if pre-commit hooks fail).
2. get prettier into a pre-commit hook
3. remove the other occurrences of prettier formatting so that it is done only once

Installing and running pre-commit hooks locally should not be mandatory, because we run them in a GitHub Actions workflow anyway.

[marcus-oscarsson]

Sounds perfect !

[fabcor-maxiv]

I now realize that while the pre-commit tool is a near de facto convention in Python world, it is far from being the case in JavaScript world and I can not seem to find an equivalent, my grand master plan is foiled! : D

I will keep investigating, there are a bunch of things I would like to improve in the GitHub Actions workflows anyway...

But if I understood correctly the issue was in the IDE settings, right? There isn't anything that needs immediate attention, is there?

[marcus-oscarsson]

Yeah, there is nothing urgent :) Thanks for having a look !