material-extensions / material-icons-browser-extension

Browser Addon that enhances file browsers of version controls with material icons.
MIT License
511 stars 38 forks source link

ESLint config is broken #32

Closed csandman closed 2 years ago

csandman commented 2 years ago

While I was working on the PR I just submitted, I noticed that ESLint wasn't working. I looked into it and it seems that there are a lot of issues with the setup. First of all, eslint-config-airbnb is specifically for react projects, what you'd want to use is eslint-config-airbnb-base. Because of that, the config is failing due to a missing dependency of eslint-plugin-jsx-a11y which is only needed for React projects.

I tried updating the config with the proper configs and plugins, but there are a ton of cases where this project goes against the rules of airbnb's config. I'd be willing to give fixing this a shot because I enjoy fixing lint errors. However, it would involve a lot of reorganizing/tweaking of the code to make it valid, so I want to make sure this is something you'd actually want before I do anything. If not you might want to either choose a more lenient ruleset, or remove eslint from the project entirely.

If you do want this to be fixed, these are the initial changes I'd probably go off of to make it work (with some tweaks along the way most likely):

.eslintrc.json

{
  "env": {
    "browser": true,
    "es2021": true,
    "node": true
  },
  "globals": {
    "chrome": "readonly"
  },
  "extends": ["airbnb-base", "prettier"],
  "parserOptions": {
    "ecmaVersion": 12,
    "sourceType": "module"
  },
  "rules": {
    "no-use-before-define": ["error", { "functions": false }]
  },
  "overrides": [
    {
      "files": ["scripts/*.js"],
      "parserOptions": {
        "sourceType": "script"
      },
      "rules": {
        "import/no-extraneous-dependencies": "off",
        "no-console": "off"
      }
    }
  ]
}

.eslintignore

dist
node_modules

package.json

"devDependencies": {
  "eslint": "8.11.0",
  "eslint-config-airbnb-base": "15.0.0",
  "eslint-config-prettier": "8.5.0",
  "eslint-plugin-import": "2.25.4",
}
csandman commented 2 years ago

Actually, I gave it a shot because I realized setting the no-use-before-define rule to not be called on function definitions to alleviated a lot of the pain points. I pushed it to a branch if you want to see the result: https://github.com/csandman/github-material-icons-extension/commit/9364b83ddcf298ce7d2a9b49b6d6ea1a8bde6fcc

For the base of the config, I just removed what you had already and ran npx eslint --init to get a new config, and tweaked the rules/overrides as needed.

I also removed the eslint-plugin-prettier package because they don't recommend using it.

Claudiohbsantos commented 2 years ago

@csandman Yeah, the linting configs on this repo have been long overdue for a fix, I just never spend long enough in it since the fixes are always fairly small so it always ends up being postponed 🙈.

Looks like I'm very late to this issue as well but I just checked your branch and it looks really good, thanks for putting time into it! If you wanna open a PR with those changes I'd be happy merge them in (given all scripts run properly and it builds, ofc)

csandman commented 2 years ago

I'll definitely give that branch another look, and I can submit a PR after we get my other one merged in! That branch will have to be updated based on all the changes made in that PR.

Claudiohbsantos commented 2 years ago

Sounds good to me! I just merged the azure/bitbucket PR to this should keep merge conflicts to a minimum

Claudiohbsantos commented 2 years ago

Closing this issue and moving discussion to #38