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

Enforce Prettier formatting? #1313

Open
fabcor-maxiv opened this issue Jul 11, 2024 · 10 comments · May be fixed by #1316
Open

Enforce Prettier formatting? #1313

fabcor-maxiv opened this issue Jul 11, 2024 · 10 comments · May be fixed by #1316

Comments

@fabcor-maxiv
Copy link
Contributor

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?

@axelboc

This comment was marked as outdated.

@marcus-oscarsson
Copy link
Member

marcus-oscarsson commented Jul 11, 2024

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
Copy link
Member

marcus-oscarsson commented Jul 11, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

axelboc commented Jul 11, 2024

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
Copy link
Collaborator

axelboc commented Jul 11, 2024

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
Copy link
Contributor Author

fabcor-maxiv commented Jul 11, 2024

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:

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
Copy link
Member

Sounds perfect !

fabcor-maxiv added a commit to fabcor-maxiv/mxcubeweb that referenced this issue Jul 11, 2024
@fabcor-maxiv fabcor-maxiv linked a pull request Jul 11, 2024 that will close this issue
fabcor-maxiv added a commit to fabcor-maxiv/mxcubeweb that referenced this issue Jul 11, 2024
fabcor-maxiv added a commit to fabcor-maxiv/mxcubeweb that referenced this issue Jul 11, 2024
fabcor-maxiv added a commit to fabcor-maxiv/mxcubeweb that referenced this issue Jul 11, 2024
@fabcor-maxiv
Copy link
Contributor Author

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
Copy link
Member

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

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 a pull request may close this issue.

3 participants