Skip to content

refactor: automatically generate all configs #358

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

Merged
merged 11 commits into from
Apr 29, 2021
Merged

refactor: automatically generate all configs #358

merged 11 commits into from
Apr 29, 2021

Conversation

MichaelDeBoey
Copy link
Member

I first tried to re-use recommended and let that be an object that has the supported testing frameworks as keys and as value either just 'error', 'warn' or false or a tuple that has one of these values as first value and the possible config as second.
This way each plugin can have totally different config options (as seen in no-dom-import.

When automatically generating all configs (see 252b0b9), I got empty rules objects as we are now hardcoding recommended to false in createTestingLibraryRule (even though the input has it in the new way).
That's why I decided to use a new recommendedConfig key in the meta docs.

Everything is working as it should be now, except for consistent-data-testid.
Since we're not using createTestingLibraryRule there, TS is complaining when adding the new recommendedConfig key in the meta docs.
I've seen why we're not using createTestingLibraryRule, but can we switch to it either way or should we look into a totally different solution?

This PR is only generating configs.
I'll create follow-up PRs for generating docs tables in README and other docs related tables in the rules specific docs once this one's merged.


Closes #187

Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

Oh, I forgot that consistent-data-testid wasn't using our custom rule creator. When I refactored that rule for v4 I originally added an option to skip the reporting checks. You can see that in the original PR. I added a new detectionOptions property to createTestingLibraryRule so we could pass skipRuleReportingCheck to avoid preventing rules to be reported with the mechanism at the very bottom of detect-testing-library-utils.ts (this wasn't implemented).

I eventually reverted the mechanism since I didn't get any advantage using our custom rule creator in that rule. I think what we can do is to get that idea back, tweaking it as we need and implementing the actual skip in detect-testing-library-utils.ts (I can give you more details if you are not really sure about this).

Apart from that, the PR looks really nice! I've left a couple of comments for some small tweaks. We also need to mention this recommendedConfig property in CONTRIBUTING.md

@MichaelDeBoey
Copy link
Member Author

@Belco90 I'll create a new PR where I refactor consistent-data-testid to work with createTestingLibraryRule too, so this PR doesn't become too big.

Once that's merged, I can rebase this one and fix all the comments.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

This is looking great! ✨

Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

Fantastic work @MichaelDeBoey 💯

The last thing would be referencing recommendedConfig in CONTRIBUTING.md file, can you add some doc about it please?

timdeschryver
timdeschryver previously approved these changes Apr 28, 2021
@Belco90 Belco90 merged commit 610b3b9 into testing-library:main Apr 29, 2021
@MichaelDeBoey MichaelDeBoey deleted the generate-configs branch April 29, 2021 10:53
@github-actions
Copy link

github-actions bot commented May 3, 2021

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickserv
Copy link
Member

nickserv commented May 6, 2021

Personally I think it's simpler to dynamically generate objects in the package, and it avoids issues with the generated files getting out of date in version control. Would you be interested in an additional PR for that?

@Belco90
Copy link
Member

Belco90 commented May 6, 2021

@nickmccurdy that's how I assumed it would work at the beginning, but I'm happy with the current implementation so we can include the generated files in the repo and check the expected config in CI. Thanks anyway!

@MichaelDeBoey
Copy link
Member Author

@nickmccurdy That's how I wanted to implement it at first.
But thinking about this, I didn't find a way to check out each config file completely (which is a use case I have often myself) so that's why I decided to go with the same way @typescript-eslint is going.

@nickserv
Copy link
Member

nickserv commented May 8, 2021

You can use eslint --print-config FILE to see the full config for any file. I personally don't think showing the config in the source code additionally is worth the maintenance burden of having to manually commit updates. Also, there is no longer a single source of truth for the config file, like duplicating props to state in a React component.

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

Successfully merging this pull request may close these issues.

Autogenerate configs from rule files
4 participants