Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_service): treat nursery rules differently #4511

Merged
merged 1 commit into from
May 19, 2023

Conversation

ematipico
Copy link
Contributor

Summary

Closes #4479

We treat nursery rules differently from the rest of the rules. We treated them differently only when using the top recommended: true but not when nursery: {} was defined.

This PR changes the code generation of the configuration to enable recommended rules only when recommended: true.

Test Plan

Added the test case of the issue

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@ematipico ematipico force-pushed the fix/nursery-rules-configuration branch from a3d1193 to 0313b47 Compare May 19, 2023 09:12
@netlify
Copy link

netlify bot commented May 19, 2023

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 0313b47
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64673d9bcfb9e500079a1259
😎 Deploy Preview https://deploy-preview-4511--docs-rometools.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project configuration and loading A-Tooling Area: our own build, development, and release tooling labels May 19, 2023
@ematipico ematipico requested a review from Conaclos May 19, 2023 09:13
@ematipico
Copy link
Contributor Author

cc @Vivalldi

@github-actions
Copy link

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47796 47796 0
Failed 1067 1067 0
Panics 0 0 0
Coverage 97.82% 97.82% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6212 6212 0
Passed 1763 1763 0
Failed 4449 4449 0
Panics 0 0 0
Coverage 28.38% 28.38% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 573 573 0
Failed 66 66 0
Panics 0 0 0
Coverage 89.67% 89.67% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13127 13127 0
Failed 4097 4097 0
Panics 0 0 0
Coverage 76.21% 76.21% 0.00%

@Conaclos
Copy link
Contributor

Conaclos commented May 19, 2023

Should we also update how we handle all? I don't think so, as the intention of all is to enable all rules, including nursery rules. However, this looks a bit inconsistent and complex. A user might expect all to behave like recommended, i.e. affecting only stable (non-nursery) rules.

I am still not a fan of the current configuration shape: too much nesting and a bit complex for my taste. I already suggested this: I might prefer a flag to enable/disable the application of recommended andall to nursery rules. Something like experimental.

In my opinion, [group], recommended, all should be tags on rules.
The default configuration could be something like { recommended: true, nursery: false }. Note that here we don't need experimental since nursery is a group/tag.
Rules could be listed directly in `rules'. This also avoids the need to update the config when a nursery rule is promoted.

@ematipico
Copy link
Contributor Author

ematipico commented May 19, 2023

True. all should not change its behaviour for now.

You might be onto something, although I don't understand yet how you would want to change the configuration.

If you want , feel free to open a discussion with your proposal :)

@ematipico ematipico merged commit ab35248 into main May 19, 2023
@ematipico ematipico deleted the fix/nursery-rules-configuration branch May 19, 2023 11:01
Copy link
Contributor

@Vivalldi Vivalldi left a comment

Choose a reason for hiding this comment

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

I appreciate all the work you've done to get this in 🙇🏻

Comment on lines +2251 to +2258
r#"const bannedType: Boolean = true;

if (true) {
const obj = {};
obj["useLiteralKey"];
}
"#,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

A drawback to this test is it relies on one of these 3 rules to be in the nursery still

lint/nursery/noBannedTypes
lint/nursery/noConstantCondition
lint/nursery/useLiteralKeys

Are there any nursery rules that only exist at test time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no. That's the bad thing of this test :(

When we promote the rules, it's going to fail, probably. But I think it's okay.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Project Area: project configuration and loading A-Tooling Area: our own build, development, and release tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Enabling one nursery rule causes all other nursery rules to be enabled
3 participants