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

Accept more types of eslint presets #96

Merged
merged 6 commits into from
Oct 8, 2020

Conversation

matt-tingen
Copy link
Contributor

This change expands the set of eslint preset names accepted by the eslint task to be closer to what is accepted by eslint for the extends configuration option including scoped packages and intra-package configs (e.g. eslint-config-airbnb/whitespace). This change does not include support for presets specified by a file path.

It's worth noting that the @eslint/eslintrc package exports a normalizePackageName function, but there are a couple issues with that:

  1. The package is explicitly marked as not for external use
  2. The function does not handle intra-package configs

@codecov-commenter
Copy link

Codecov Report

Merging #96 into master will decrease coverage by 0.08%.
The diff coverage is 93.75%.

Impacted Files Coverage Δ
packages/mrm-task-eslint/index.js 98.59% <93.75%> (-1.41%) ⬇️

Copy link
Owner

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!

const match = presetName.match(presetNameRegex);

if (!match) {
throw new Error('Invalid preset name');
Copy link
Owner

Choose a reason for hiding this comment

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

Let's make it more clear for the user:

Suggested change
throw new Error('Invalid preset name');
throw new Error(`Invalid preset name is passed to the eslint task: ${presetName}`);

Comment on lines 20 to 32
const scope = match[1] || '';
let configName = match[2];

if (!scope && !configName.startsWith(prefix)) {
configName = `${prefix}-${configName}`;
} else if (scope && !configName) {
configName = prefix;
}

const packageName = `${scope ? `${scope}/` : ''}${configName}`;

return packageName;
};
Copy link
Owner

Choose a reason for hiding this comment

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

Let's extract this piece to a function to avoid reassignment:

const [, scope = '', configNameRaw] = match;

// ...

const getConfigName = (configName, scope, prefix) => {
if (!scope && !configName.startsWith(prefix)) {
		return `${prefix}-${configName}`;
	} else if (scope && !configName) {
		return prefix;
	} else {
return configName;
}
}

Comment on lines 51 to 52
const presetPackage = normalizePresetPackageName(eslintPreset);
packages.push(presetPackage);
Copy link
Owner

Choose a reason for hiding this comment

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

A variable doesn't improve readability here:

Suggested change
const presetPackage = normalizePresetPackageName(eslintPreset);
packages.push(presetPackage);
packages.push(normalizePresetPackageName(eslintPreset));

@matt-tingen matt-tingen requested a review from sapegin October 7, 2020 22:49
@codecov-io
Copy link

Codecov Report

Merging #96 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
packages/mrm-task-eslint/index.js 100.00% <100.00%> (ø)

@sapegin sapegin merged commit 1f72975 into sapegin:master Oct 8, 2020
@sapegin
Copy link
Owner

sapegin commented Oct 8, 2020

Thanks, merging! 🦄

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.

4 participants