Skip to content
This repository was archived by the owner on Jul 11, 2024. It is now read-only.

refactor: create recommended config based on rules config #35

Merged
merged 4 commits into from
Jun 23, 2020

Conversation

timdeschryver
Copy link
Owner

Thoughts @jsaguet ?

@codecov
Copy link

codecov bot commented Jun 21, 2020

Codecov Report

Merging #35 into master will decrease coverage by 0.65%.
The diff coverage is 87.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
- Coverage   97.51%   96.85%   -0.66%     
==========================================
  Files          17       19       +2     
  Lines         161      191      +30     
  Branches        8       12       +4     
==========================================
+ Hits          157      185      +28     
  Misses          4        4              
- Partials        0        2       +2     
Impacted Files Coverage Δ
src/index.ts 0.00% <0.00%> (ø)
src/rules/action-hygiene.ts 100.00% <ø> (ø)
src/utils/helper-functions/index.ts 87.50% <82.35%> (-12.50%) ⬇️
src/configs/recommended.ts 100.00% <100.00%> (ø)
src/rules/index.ts 100.00% <100.00%> (ø)
src/rules/no-multiple-actions-in-effects.ts 100.00% <100.00%> (ø)
src/utils/selectors/index.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8afd40a...cf3bf63. Read the comment docs.

@jsaguet
Copy link
Collaborator

jsaguet commented Jun 21, 2020

Hi @timdeschryver,
I think it's great to automate the recommended config creation but I am not sure about this implementation.

It looks like this change would make eslint "calculate" all recommended rules at runtime.

Does this work when the plugin is integrated in a project ?
If yes, it probably has some performance cost for end users for every time eslint is run on one file or their entire project.

I've seen @typescript-eslint package has a script in their package.json to generate the file.
The script itself is basically a more complicated version of your implementation. Available here

I believe we can see multiple advantages:

  • You can run it manually or on pre-commit hook for example. The test on the recommended config would catch if the script was not run.
  • It adds no further computation time for end users.
  • It's more readable for anyone looking through the repository to understand which rules are included in the recommended one.
  • You have better separation of concerns: the recommended file lists the rules (as it's supposed to) and your automation task is in a separate file.

What do you think?

@timdeschryver
Copy link
Owner Author

@jsaguet that's great feedback, thanks!
I do think that performance wise it doesn't make a difference, it's just an iteration over a small object.
I agree that it will be more clear and clean if we move it to a hook! I will look into that later.

@timdeschryver timdeschryver force-pushed the pr/automatic-recommended branch from 42a560d to 0535ada Compare June 22, 2020 16:23
Copy link
Collaborator

@jsaguet jsaguet left a comment

Choose a reason for hiding this comment

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

Nice !

@timdeschryver timdeschryver changed the title refactor: automatically create recommended config based on rule config refactor: create recommended config based on rules config Jun 23, 2020
@timdeschryver timdeschryver merged commit ce00066 into master Jun 23, 2020
@timdeschryver timdeschryver deleted the pr/automatic-recommended branch June 23, 2020 16:25
@github-actions
Copy link

🎉 This PR is included in version 1.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants