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

ESLint config: Print config and add tsdoc plugin #9740

Merged
merged 12 commits into from
Apr 6, 2022

Conversation

tylerbutler
Copy link
Member

@tylerbutler tylerbutler commented Apr 4, 2022

Part of #9501.

One question that comes up often when we make changes to our lint config is, "what changed?" This applies even when we don't make any changes other than upgrading deps, because the dependency upgrade might include a new rule.

ESLint provides a way to print the config that would apply to a file (--print-config), so this PR uses this capability to print out the applied config as a JSON file. As we make changes to the config, we can print out the config again and get a diff to review as part of a PR -- just like we do with API reports for code changes.

This PR also fixes a bug in the strict and recommended configs -- there was a missing dependency that needed to be added.

@tylerbutler tylerbutler requested review from msfluid-bot and a team as code owners April 4, 2022 21:49
@github-actions github-actions bot added the base: main PRs targeted against main branch label Apr 4, 2022
@tylerbutler tylerbutler mentioned this pull request Apr 4, 2022
8 tasks
Copy link
Member

@markfields markfields left a comment

Choose a reason for hiding this comment

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

Great idea! I see there are 2 fake ts files checked in, one under /test/, I'm assuming that's because different rules apply to test v. code files. And there's also 3 different configs. So should we have 6 baselines checked in for all the combinations?

@tylerbutler
Copy link
Member Author

Great idea! I see there are 2 fake ts files checked in, one under /test/, I'm assuming that's because different rules apply to test v. code files. And there's also 3 different configs. So should we have 6 baselines checked in for all the combinations?

Per some feedback from @ChumpChief (#9489 (comment)), I'm consolidating the disparate configs into a single config in my next PR. So I would rather just print the default config, which is the one 99% of our packages use today, and then I'll update it with the results of the config changes in my next PR.

@github-actions github-actions bot added the area: build Build related issues label Apr 5, 2022
@tylerbutler
Copy link
Member Author

@markfields I just pushed an update that includes changes to the readme, a second config for test files, and updates CI to fail the build if a config change is made without printing the config. Can you take a look? Thanks!

This was referenced Apr 5, 2022
@@ -0,0 +1,6 @@
/*!
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need two dummy files for this? can both just point to the same dummy?

Copy link
Member Author

Choose a reason for hiding this comment

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

ESLint overrides are based on glob paths -- so in order to get ESLint to correctly apply the test-only overrides, the file being linted has to match the test override globs. So it either has to be a .spec.ts file or in the /test/ path. I haven't come up with a better solution :(

@tylerbutler tylerbutler merged commit f6b94c0 into microsoft:main Apr 6, 2022
@tylerbutler tylerbutler deleted the lint/print-config branch April 6, 2022 00:15
// Sort the JSON in-place
sortJson.overwrite(filePath, { indentSize: 4 });
}
})();
Copy link
Member

Choose a reason for hiding this comment

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

No .catch to log to console or anything?

@@ -64,7 +64,7 @@ extends:
releaseBuildOverride: ${{ parameters.releaseBuildOverride }}
buildDirectory: common/build/eslint-config-fluid
tagName: eslint-config-fluid
taskBuild: false
taskBuild: build
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member

@markfields markfields left a comment

Choose a reason for hiding this comment

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

I can't click Approve from my phone for some reason. But consider it approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Build related issues base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants