AriPerkkio / eslint-remote-tester-run-action

Github action for running eslint-remote-tester and receiving results in Github issue
1 stars 1 forks source link

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

Open MichaelDeBoey opened 3 years ago

MichaelDeBoey commented 3 years ago

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 https://github.com/testing-library/eslint-plugin-testing-library/issues/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.

AriPerkkio commented 3 years ago

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.