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

#1870 fix pre commit errors #1884

Merged
merged 4 commits into from
Feb 14, 2024
Merged

#1870 fix pre commit errors #1884

merged 4 commits into from
Feb 14, 2024

Conversation

elijah0kello
Copy link
Contributor

@elijah0kello elijah0kello commented Feb 13, 2024

Description

Migrated current .eslintrc.js to flat config i.e eslint.config.js to fix the errors that were faced during running the eslint pre-commit hook.

Fixes #1870

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I have some doubts regarding the ESLint configuration. The old config file states that it extends eslint:recommended but the new one does not.

This may help: https://eslint.org/docs/latest/use/configure/configuration-files-new#using-predefined-configurations but it's an additional dependency and that also means that we always have to keep additional_dependencies updated in the pre-commit configuration (at least I don't know a better way)

A different approach might be to copy the configuration from https://github.com/eslint/eslint/blob/main/packages/js/src/configs/eslint-recommended.js into our config, but that would have to be kept up-to-date manually as well.

@elijah0kello
Copy link
Contributor Author

elijah0kello commented Feb 13, 2024

Thanks for working on this!

I have some doubts regarding the ESLint configuration. The old config file states that it extends eslint:recommended but the new one does not.

This may help: https://eslint.org/docs/latest/use/configure/configuration-files-new#using-predefined-configurations but it's an additional dependency and that also means that we always have to keep additional_dependencies updated in the pre-commit configuration (at least I don't know a better way)

A different approach might be to copy the configuration from https://github.com/eslint/eslint/blob/main/packages/js/src/configs/eslint-recommended.js into our config, but that would have to be kept up-to-date manually as well.

Thanks for the feedback. I agree there's something missing in the new config

I actually faced some challenges adding the dependencies

This is how the config file looks like with the eslint:recommended in the new flat config format

const js = require("@eslint/js");

module.exports = [
     js.configs.recommended,
    {
        languageOptions:{
            ecmaVersion: 6,
            sourceType: "module",
        }
    },
    {
        rules: {
            "curly": ["error", "all"],
            "dot-notation": "error",
            "eqeqeq": "error",
            "no-eval": "error",
            "no-var": "error",
            "prefer-const": "error",
            "semi": "error"
        }
    }
];

The issue I get though is that I can't seem to find a way to add the dependency

I tried doing so via additional_dependencies but I think I was putting a wrong dependency name to cater for the import.

This is what I added to the additional_dependencies key inside the eslint hook

    hooks:
    -   id: eslint
        additional_dependencies:
            - eslint@v9.0.0-beta.0

But when I tried to run the eslint hook, it still returns an error that module not found @eslint/js

ESLint: 9.0.0-beta.0

Error: Cannot find module '@eslint/js'
Require stack:
- /Users/macbook/Desktop/code/djangonautspace/django-debug-toolbar/eslint.config.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1149:15)
    at Module._load (node:internal/modules/cjs/loader:990:27)
    at Module.require (node:internal/modules/cjs/loader:1237:19)
    at require (node:internal/modules/helpers:176:18)
    at Object.<anonymous> (/Users/macbook/Desktop/code/djangonautspace/django-debug-toolbar/eslint.config.js:1:12)
    at Module._compile (node:internal/modules/cjs/loader:1378:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1437:10)
    at Module.load (node:internal/modules/cjs/loader:1212:32)
    at Module._load (node:internal/modules/cjs/loader:1028:12)
    at cjsLoader (node:internal/modules/esm/translators:359:17)

I'd appreciate any advise rendered.

@matthiask
Copy link
Member

I'm not sure but I think you have to add additional additional_dependencies, maybe like this:

    hooks:
    -   id: eslint
        additional_dependencies:
            - eslint@v9.0.0-beta.0
            - @eslint/js@v9.0.0-beta.0

This may work but it also does suck a bit since now dependabot will not update all those entries; it only updates the top-level version/tag.

@elijah0kello
Copy link
Contributor Author

I have tried that but pre-commit doesn't accept the @ infront of an entry in the additional_dependencies. Let me try something else and get back to you. If you have anything else in mind. Please share.

@elijah0kello elijah0kello changed the title Fix pre commit errors #1870 fix pre commit errors Feb 13, 2024
@matthiask
Copy link
Member

@elijah0kello
Copy link
Contributor Author

Thanks. This worked. Now I am going to fix the globals. That's something else that is not catered for in the current config

@elijah0kello
Copy link
Contributor Author

elijah0kello commented Feb 13, 2024

@matthiask I noticed something I think you should know. When I changed the ecmaVersion to 2022 the linting passed but when set to 6 it fails complaining about a syntax error in the eslint.config.js

  12:17  error  Parsing error: Unexpected token ..

✖ 1 problem (1 error, 0 warnings)

It supposedly doesn't recognise the spread operator

@matthiask
Copy link
Member

Yeah. ecmaVersion:6 is very outdated.

@elijah0kello
Copy link
Contributor Author

I have set the ecmaVersion to "latest". Hope that is fine

@matthiask
Copy link
Member

Thanks! I'm very happy with this update. I'm not totally happy that we have to spell out the ESLint dependency versions now, but there's not much you can do about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pre-commit Hooks: ESLint 9 will change the way eslint is configured
3 participants