jazzband / django-debug-toolbar

A configurable set of panels that display various debug information about the current request/response.
https://django-debug-toolbar.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
8.07k stars 1.05k forks source link

#1870 fix pre commit errors #1884

Closed elijah0kello closed 7 months ago

elijah0kello commented 7 months ago

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:

elijah0kello commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.

matthiask commented 7 months ago

Adding string quotes should work:

https://github.com/feincms/feincms3/blob/bec2c6331ef297a7e101e890dbb8fd44209547ba/.pre-commit-config.yaml#L36-L45

elijah0kello commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

Yeah. ecmaVersion:6 is very outdated.

elijah0kello commented 7 months ago

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

matthiask commented 7 months ago

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.