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

Deep merge provided config with a default config that makes sense in a CI environment #9

Open
MichaelDeBoey opened this issue Apr 25, 2021 · 1 comment

Comments

@MichaelDeBoey
Copy link
Contributor

MichaelDeBoey commented Apr 25, 2021

When looking at the configs of eslint-plugin-unicorn, eslint-plugin-jest-dom, eslint-plugin-knex and the proposed config for eslint-plugin-react, I see a lot of config options that are exactly the same for each repo that needs to be maintained by all of them.

Having a package like eslint-remote-tester-repositories that already provides the repositories & pathIgnorePattern is a good first step, but I think we can go even further.

As discussed in testing-library/eslint-plugin-testing-library#341, I would suggest to have a default config that makes sense in a CI environment (which we always are, as this is the run-action repo) and that we deep merge that with the provided config.

  • default cache to false (fix: disable cache by default #10)
  • default ci to true (feat: enable CI by default #11)
    This is already the default of eslint-remote-tester (when process.env.CI === 'true'), so we can even delete it from all custom configs
  • default concurrentTasks to 3
    Maybe even change the default value in eslint-remote-tester from 5 to 3? 🤔
  • default extensions to ['js', 'jsx', 'ts', 'tsx']
    This should even be the default in eslint-remote-tester I think? 🤔
  • default pathIgnorePattern to the one from eslint-remote-tester-repositories
    This should even be the default in eslint-remote-tester I think? 🤔
  • default repositories to the ones from eslint-remote-tester-repositories
    This should even be the default in eslint-remote-tester I think? 🤔
  • default rulesUnderTesting to [] (feat: default rulesUnderTesting to [] #12)
    This is already the default of eslint-remote-tester, so we can even delete it from all custom configs
  • default eslintrc to the following:
    eslintrc: {
      root: true,
      env: {
        es6: true
      },
      parser: '@typescript-eslint/parser',
      parserOptions: {
        ecmaVersion: 2020,
        sourceType: 'module',
        ecmaFeatures: {
          jsx: true,
        },
      },
      settings: {
        react: {
          version: '16.13.1',
        },
      },
    },
    So plugins using this action should only provide
    eslintrc: {
      extends: ['plugin:rule-config'],
    },
    or
    eslintrc: {
      plugins: ['plugin-name'],
      rules: {
         // ...
      },
    },
    But they can still override all of these if they want to.
@AriPerkkio
Copy link
Owner

Thanks for raising this issue! There are some really good improvements here.

default cache to false

Adding this as default value in eslint-remote-tester-run-action sounds good.

default ci to true
[...], so we can even delete it from all custom configs

Yes, this should be removed from all CI configurations.

default concurrentTasks to 3
Maybe even change the default value in eslint-remote-tester from 5 to 3? 🤔

I'm OK for defaulting this as 3 in eslint-remote-tester-run-action. Default value of eslint-remote-tester attempts to match CPUs of local development machines. Github CI is using 2-core CPUs which is quite limited.

default extensions to ['js', 'jsx', 'ts', 'tsx']
This should even be the default in eslint-remote-tester I think? 🤔

Sounds good. This is something I'm always copy-pasting everywhere.
@MichaelDeBoey it would be great if you could implement this in eslint-remote-tester. I would really like to see someone diving into its code base and see if making changes is easy.

default pathIgnorePattern to the one from eslint-remote-tester-repositories
default repositories to the ones from eslint-remote-tester-repositories
This should even be the default in eslint-remote-tester I think? 🤔

These are a bit tricky. If these were used by eslint-remote-tester-run-action how would a user ignore specific repository? Some plugins might want to ignore repositories which are utilizing some experimental language/syntax features.

Should these be default in eslint-remote-tester? I'm not sure. This would be quite big assumption.

But I do want to make it easier to setup repositories - just not sure how. eslint-remote-tester-repositories is the first attempt to solve this.

default rulesUnderTesting to []
This is already the default of eslint-remote-tester, so we can even delete it from all custom configs

Yes, this should be removed from configurations.

default eslintrc to the following: [...]

I believe settings.react is really just eslint-plugin-react specific configuration.

The parser: '@typescript-eslint/parser' is something we can't really default to. While typescript parser is popular some might prefer babel parser. I'm also not sure how this peer dependency would be informed properly.

Those other eslintrc options might be a good default.

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

No branches or pull requests

2 participants