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

[IMP] pre-commmit: Run prettier and eslint as local hooks and update versions in v18 #278

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

PabloEForgeFlow
Copy link
Contributor

Should fix #264 and #224.

@PabloEForgeFlow PabloEForgeFlow force-pushed the update-pre-commit branch 2 times, most recently from 3635480 to d12dbc6 Compare October 9, 2024 06:44
@PabloEForgeFlow
Copy link
Contributor Author

@sbidoul I made the prettier and eslint hooks local and updated versions at the same time. Perhaps they should be two separate PRs?

Also, pre-commit was failing, due to the .prettierrc.yml symlink being broken, so I updated it to the new '{% if 12 < odoo_version < 18 %}.prettierrc.yml{% endif %}', but we'll probably want to use the v18 file once we're sure it's working.

@sbidoul
Copy link
Member

sbidoul commented Oct 10, 2024

Is it not necessary to convert eslintrc.yml to cjs, and to the new eslint 9 config format for v18?

We could also upgrade node for v18.

BTW, I wonder why GitHub is still trying to run the tests for 11 and 12.

@PabloEForgeFlow
Copy link
Contributor Author

Is it not necessary to convert eslintrc.yml to cjs, and to the new eslint 9 config format for v18?

Will do that, I thought it was something limited to prettier, but it does appear to have changed as well. Guess that's the new way of doing configuration files in the javascript world.

We could also upgrade node for v18.

I will look into it, see what version makes most sense.

BTW, I wonder why GitHub is still trying to run the tests for 11 and 12.

Yeah that's weird, maybe something's still cached on GitHub's side?

@PabloEForgeFlow PabloEForgeFlow force-pushed the update-pre-commit branch 3 times, most recently from 779ac1d to 5562915 Compare October 11, 2024 07:52
@PabloEForgeFlow
Copy link
Contributor Author

PabloEForgeFlow commented Oct 11, 2024

I've migrated the eslint configuration to the new format following both eslint and jsdoc guides. Most parts have been migrated using the configuration migrator, but some, such as jsdoc settings, were done by hand. I also set ecmaVersion to 2024, but I'm not sure if we would rather stay on 2022.

I also update the node to version 22, which is the latest TLS release, and fixed some issues with prettier.

@sbidoul
Copy link
Member

sbidoul commented Oct 11, 2024

Hi @PabloEForgeFlow I did a test by applying the template from your PR onto OCA/storage#399

I noticed two things:

  1. In 18+, we need to remove .prettierrc.yml otherwise it takes precedence over the cjs config. We also need to remove the old .eslintrc.yml. This can be done in copier.yml, _tasks.
  2. I think we need to exclude ^eslint.config.cjs|^prettier.config.cjs, otherwise eslint is trying to check them and is not happy with what it sees.

@PabloEForgeFlow
Copy link
Contributor Author

Makes sense, I added a line to _tasks removing them and put both in the global pre-commit exclude.

@sbidoul sbidoul merged commit 55aaa4f into OCA:master Oct 14, 2024
7 checks passed
@@ -5,6 +5,7 @@ _min_copier_version: "9"

_tasks:
- rm -f {% if ci != "Travis" %}.travis.yml{% endif %} .t2d.yml
- rm -f .prettierrc.yml .eslintrc.yml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PabloEForgeFlow these should be removed only for odoo_version >= 18, right? Can you adjust that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6f0c788

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though they would be recreated, but yeah it's probably more clear this way.

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.

pre-commit/mirrors-prettier has been archived
2 participants