Skip to content

NEW: @W-18495437@: Detect state of user's eslint configuration files … #292

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 2 commits into from
May 15, 2025

Conversation

stephen-carter-at-sf
Copy link
Collaborator

…and emit fine log

const WORKSPACE_WITH_FLAT_CONFIG2: string = path.join(TEST_DATA_FOLDER, 'workspaceWithFlatConfig2');
const WORKSPACE_WITH_FLAT_CONFIG3: string = path.join(TEST_DATA_FOLDER, 'workspaceWithFlatConfig3');

describe('Tests for the UserConfigInfo class', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file is rather exhaustive to cover all the different scenarios for the most part.

Comment on lines +103 to +104
IgnoringFlatConfigFile:
`Ignoring '%s' since ESLint "Flat" configuration files are not yet supported by this version of the 'eslint' engine.`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that this doesn't actually get displayed unless the file is auto discovered when auto_discover_eslint_config is enabled. The config still errors today when a user tries to explicitely set a flat config file.

// For now we just maintain a test that the v9 eslint engine errors until it has been implemented:
it('When directly calling describeRules on the v9 ESLintEngine, then we error since it has not been implemented', async () => {
const engine: ESLintEngine = new ESLintEngine(DEFAULT_CONFIG);
// For now we just maintain a test that the v9 eslint engine defers when auto discovered flat config is discovered until it has been implemented:
Copy link
Contributor

Choose a reason for hiding this comment

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

"Defers" is unclear to me. What's it deferring to? V8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. I can update this to make it more clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the only difference between these three workspaces is the file extension of the config. Could we rename the workspaces to workspaceWithJsConfig, workspaceWithMjsConfig, and workspaceWithCjsConfig for clarity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that is ambiguous though. js and cjs files can either be legacy or flat.
But yeah I could rename things to be workspaceWithJsFlatConfig, workspaceWithCjsFlatConfig, workspaceWithMjsFlatConfig but the workspaceWithFlatConfig3 also has a legacy eslint ignore file in it so I can test that case as well. So I don't know how specific I want to make each of these scenarios. I was just thinking of scenario 1, 2, and 3. And I'll be adding to these soon - so it isn't just that 1 is js based, 2 is cjs based, and 3 is mjs based. I just wanted to have 3 scenarios.

Notice that in the LegacyConfig1, LegacyConfig2, and LegacyConfig3 cases I could have kept going with 4, 5, 6 etc... because I'm not even testing "yaml" for example. It's not fully exhaustive. It just gives me 3 slots to sprinkle in differences as needed moving forward. This sprinkling strategy avoids having to have a million test scenarios. So hopefully we'll be able to stick to 3 each and I can sprinkle what I need into these 3 to cover nearly all cases that actually might catch bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. If we can't have more detail in the names of the folders, could we maybe have a README in each folder that describes the scenarios it covers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just did the rename for now. Numbers, extensions... all the same to me.

@stephen-carter-at-sf stephen-carter-at-sf merged commit 4274384 into dev May 15, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants