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.

Sign up for GitHub

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

Jump to bottom

Rework error collection, error if tslint.json is present #950

Merged
merged 4 commits into from
Feb 12, 2024
Merged

Rework error collection, error if tslint.json is present #950

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69e8b39
Rework error collection
jakebailey Feb 12, 2024
52fbd83
Error if tslint.json is present
jakebailey Feb 12, 2024
2cf720d
Merge branch 'main' into error-redo-more
jakebailey Feb 12, 2024
d881fb0
Fix error
jakebailey Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/chatty-jobs-pull.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@definitelytyped/dtslint": patch
---

Rework error collection
5 changes: 5 additions & 0 deletions .changeset/fifty-points-eat.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@definitelytyped/dtslint": patch
---

Error if tslint.json is present
32 changes: 16 additions & 16 deletions packages/dtslint/src/checks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,11 @@
configPath: string,
expectError: boolean,
): Promise<{
warnings?: string[];
errors?: string[];
warnings: string[];
errors: string[];
}> {
let warnings: string[] | undefined;
let errors: string[] | undefined;
const warnings = [];
const errors = [];
let result: {
status: "pass" | "fail" | "error";
output: string;
Expand Down Expand Up @@ -207,12 +207,12 @@
break;
case "fail":
// Show output without failing the build.
(warnings ??= []).push(
warnings.push(
`Ignoring attw failure because "${dirName}" is listed in 'failingPackages'.\n\n@arethetypeswrong/cli\n${output}`,
);
break;
case "pass":
(errors ??= []).push(`attw passed: remove "${dirName}" from 'failingPackages' in attw.json\n\n${output}`);
errors.push(`attw passed: remove "${dirName}" from 'failingPackages' in attw.json\n\n${output}`);
break;
default:
assertNever(status);
Expand All @@ -221,7 +221,7 @@
switch (status) {
case "error":
case "fail":
(errors ??= []).push(`!@arethetypeswrong/cli\n${output}`);
errors.push(`!@arethetypeswrong/cli\n${output}`);
break;
case "pass":
// Don't show anything for passing attw - most lint rules have no output on success.
Expand All @@ -236,12 +236,12 @@
packageJson: header.Header,
packageDirectoryNameWithVersion: string,
): Promise<{
warnings?: string[];
errors?: string[];
warnings: string[];
errors: string[];
implementationPackage?: attw.Package;
}> {
let warnings: string[] | undefined;
let errors: string[] | undefined;
const warnings: string[] = []
const errors: string[] = [];
let hasNpmVersionMismatch = false;
let implementationPackage;
const attw = await import("@arethetypeswrong/core");
Expand All @@ -252,7 +252,7 @@
if (packageId) {
const { packageName, packageVersion, tarballUrl } = packageId;
if (packageJson.nonNpm === true) {
(errors ??= []).push(
errors.push(
`Package ${packageJson.name} is marked as non-npm, but ${packageName} exists on npm. ` +
`If these types are being added to DefinitelyTyped for the first time, please choose ` +
`a different name that does not conflict with an existing npm package.`,
Expand All @@ -261,7 +261,7 @@
if (!satisfies(packageVersion, typesPackageVersion)) {
hasNpmVersionMismatch = true;
const isError = !npmVersionExemptions.has(packageDirectoryNameWithVersion);
const container = isError ? (errors ??= []) : (warnings ??= []);
const container = isError ? errors : warnings;
container.push(
(isError
? ""
Expand All @@ -278,21 +278,21 @@
implementationPackage = await attw.createPackageFromTarballUrl(tarballUrl);
}
}
} else if (packageJson.nonNpm === "conflict") {

Check failure on line 281 in packages/dtslint/src/checks.ts

View workflow job for this annotation

GitHub Actions / build and test 

Cannot assign to 'warnings' because it is a constant.
(errors ??= []).push(
errors.push(
`Package ${packageJson.name} is marked as \`"nonNpm": "conflict"\`, but no conflicting package name was ` +
`found on npm. These non-npm types can be makred as \`"nonNpm": true\` instead.`,
);
} else if (!packageJson.nonNpm) {
(errors ??= []).push(
errors.push(
`Package ${packageJson.name} is not marked as non-npm, but no implementation package was found on npm. ` +
`If these types are not for an npm package, please add \`"nonNpm": true\` to the package.json. ` +
`Otherwise, ensure the name of this package matches the name of the npm package.`,
);
}

if (!hasNpmVersionMismatch && npmVersionExemptions.has(packageDirectoryNameWithVersion)) {
(warnings ??= []).push(
warnings.push(
`${packageDirectoryNameWithVersion} can be removed from expectedNpmVersionFailures.txt in https://github.com/microsoft/DefinitelyTyped-tools/blob/main/packages/dtslint.`,
);
}
Expand Down
114 changes: 63 additions & 51 deletions packages/dtslint/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ async function runTests(
expectOnly: boolean,
npmChecks: boolean | "only",
tsLocal: string | undefined,
): Promise<string | undefined> {
): Promise<string> {
// Assert that we're really on DefinitelyTyped.
const dtRoot = findDTRoot(dirPath);
const packageName = packageNameFromPath(dirPath);
Expand All @@ -174,25 +174,34 @@ async function runTests(
throw new Error("\n\t* " + packageJson.join("\n\t* "));
}

await assertNpmIgnoreExpected(dirPath);
assertNoOtherFiles(dirPath);
const warnings: string[] = [];
const errors: string[] = [];

let implementationPackage;
let warnings: string[] | undefined;
let errors: string[] | undefined;

if (npmChecks) {
({ implementationPackage, warnings, errors } = await checkNpmVersionAndGetMatchingImplementationPackage(
const result = await checkNpmVersionAndGetMatchingImplementationPackage(
packageJson,
packageDirectoryNameWithVersion,
));
);

implementationPackage = result.implementationPackage;
warnings.push(...result.warnings);
errors.push(...result.errors);
}

if (npmChecks !== "only") {
const minVersion = maxVersion(packageJson.minimumTypeScriptVersion, TypeScriptVersion.lowest);
if (onlyTestTsNext || tsLocal) {
const tsVersion = tsLocal ? "local" : TypeScriptVersion.latest;
await testTypesVersion(dirPath, tsVersion, tsVersion, expectOnly, tsLocal, /*isLatest*/ true);
const testTypesResult = await testTypesVersion(
dirPath,
tsVersion,
tsVersion,
expectOnly,
tsLocal,
/*isLatest*/ true,
);
errors.push(...testTypesResult.errors);
} else {
// For example, typesVersions of [3.2, 3.5, 3.6] will have
// associated ts3.2, ts3.5, ts3.6 directories, for
Expand All @@ -213,7 +222,8 @@ async function runTests(
if (lows.length > 1) {
console.log("testing from", low, "to", hi, "in", versionPath);
}
await testTypesVersion(versionPath, low, hi, expectOnly, undefined, isLatest);
const testTypesResult = await testTypesVersion(versionPath, low, hi, expectOnly, undefined, isLatest);
errors.push(...testTypesResult.errors);
}
}
}
Expand All @@ -224,8 +234,8 @@ async function runTests(
const dirName = dirPath.slice(dtRoot.length + "/types/".length);
const expectError = !!failingPackages?.includes(dirName);
const attwResult = await runAreTheTypesWrong(dirName, dirPath, implementationPackage, attwJson, expectError);
(warnings ??= []).push(...(attwResult.warnings ?? []));
(errors ??= []).push(...(attwResult.errors ?? []));
warnings.push(...attwResult.warnings);
errors.push(...attwResult.errors);
}

const result = combineErrorsAndWarnings(errors, warnings);
Expand All @@ -235,15 +245,9 @@ async function runTests(
return result;
}

function combineErrorsAndWarnings(
errors: string[] | undefined,
warnings: string[] | undefined,
): Error | string | undefined {
if (!errors && !warnings) {
return undefined;
}
const message = (errors ?? []).concat(warnings ?? []).join("\n\n");
return errors?.length ? new Error(message) : message;
function combineErrorsAndWarnings(errors: string[], warnings: string[]): Error | string {
const message = errors.concat(warnings).join("\n\n");
return errors.length ? new Error(message) : message;
}

function maxVersion(v1: AllTypeScriptVersion, v2: TypeScriptVersion): TypeScriptVersion {
Expand All @@ -265,16 +269,19 @@ async function testTypesVersion(
expectOnly: boolean,
tsLocal: string | undefined,
isLatest: boolean,
): Promise<void> {
assertIndexdts(dirPath);
): Promise<{ errors: string[] }> {
const errors = [];
const checkExpectedFilesResult = checkExpectedFiles(dirPath, isLatest);
Copy link
Member Author

Choose a reason for hiding this comment

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

This check is now happening per types version, since those folders can also have lint configs... This code may not run if this kind of testing is disabled, e.g. npmChecksOnly mode, which is unfortunate. Was not sure how to clean be able to run on each versioned dir, though.

errors.push(...checkExpectedFilesResult.errors);
const tsconfigErrors = checkTsconfig(dirPath, getCompilerOptions(dirPath));
if (tsconfigErrors.length > 0) {
throw new Error("\n\t* " + tsconfigErrors.join("\n\t* "));
errors.push("\n\t* " + tsconfigErrors.join("\n\t* "));
Copy link
Member

Choose a reason for hiding this comment

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

It may soon be worth collecting structured error data and making a nice reporter/formatter, or using a dependency if you know of a good one. Our newlines and indentation are pretty inconsistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I saw this and was like "Oh yeah this has some pretty formatting, doesn't it".

From what I can tell, this eventually gets thrown as an error, so as long as it adds a leading newline, it actually turns out okay. But we don't do that in many cases.

}
const err = await lint(dirPath, lowVersion, hiVersion, isLatest, expectOnly, tsLocal);
if (err) {
throw new Error(err);
errors.push(err);
}
return { errors };
}

function findDTRoot(dirPath: string) {
Expand Down Expand Up @@ -336,42 +343,47 @@ if (require.main === module) {
});
}

async function assertNpmIgnoreExpected(dirPath: string) {
const expected = ["*", "!**/*.d.ts", "!**/*.d.cts", "!**/*.d.mts", "!**/*.d.*.ts"];
function checkExpectedFiles(dirPath: string, isLatest: boolean): { errors: string[] } {
const errors = [];

if (basename(dirname(dirPath)) === "types") {
for (const subdir of fs.readdirSync(dirPath, { withFileTypes: true })) {
if (subdir.isDirectory() && /^v(\d+)(\.(\d+))?$/.test(subdir.name)) {
expected.push(`/${subdir.name}/`);
if (isLatest) {
const expectedNpmIgnore = ["*", "!**/*.d.ts", "!**/*.d.cts", "!**/*.d.mts", "!**/*.d.*.ts"];

if (basename(dirname(dirPath)) === "types") {
for (const subdir of fs.readdirSync(dirPath, { withFileTypes: true })) {
if (subdir.isDirectory() && /^v(\d+)(\.(\d+))?$/.test(subdir.name)) {
expectedNpmIgnore.push(`/${subdir.name}/`);
}
}
}
}

const expectedString = expected.join("\n");
const expectedNpmIgnoreAsString = expectedNpmIgnore.join("\n");
const npmIgnorePath = joinPaths(dirPath, ".npmignore");
if (!fs.existsSync(npmIgnorePath)) {
errors.push(`${dirPath}: Missing '.npmignore'; should contain:\n${expectedNpmIgnoreAsString}`);
}

const npmIgnorePath = joinPaths(dirPath, ".npmignore");
if (!fs.existsSync(npmIgnorePath)) {
throw new Error(`${dirPath}: Missing '.npmignore'; should contain:\n${expectedString}`);
}
const actualNpmIgnore = fs.readFileSync(npmIgnorePath, "utf-8").trim().split(/\r?\n/);
if (!deepEquals(actualNpmIgnore, expectedNpmIgnore)) {
errors.push(`${dirPath}: Incorrect '.npmignore'; should be:\n${expectedNpmIgnoreAsString}`);
}

const actualRaw = await fs.promises.readFile(npmIgnorePath, "utf-8");
const actual = actualRaw.trim().split(/\r?\n/);
if (fs.existsSync(joinPaths(dirPath, "OTHER_FILES.txt"))) {
errors.push(
`${dirPath}: Should not contain 'OTHER_FILES.txt'. All files matching "**/*.d.{ts,cts,mts,*.ts}" are automatically included.`,
);
}
}

if (!deepEquals(actual, expected)) {
throw new Error(`${dirPath}: Incorrect '.npmignore'; should be:\n${expectedString}`);
if (!fs.existsSync(joinPaths(dirPath, "index.d.ts"))) {
errors.push(`${dirPath}: Must contain 'index.d.ts'.`);
}
}

function assertNoOtherFiles(dirPath: string) {
if (fs.existsSync(joinPaths(dirPath, "OTHER_FILES.txt"))) {
throw new Error(
`${dirPath}: Should not contain 'OTHER_FILES.txt'. All files matching "**/*.d.{ts,cts,mts,*.ts}" are automatically included.`,
if (fs.existsSync(joinPaths(dirPath, "tslint.json"))) {
errors.push(
`${dirPath}: Should not contain 'tslint.json'. This file is no longer required; place all lint-related options into .eslintrc.json.`,
);
}
}

function assertIndexdts(dirPath: string) {
if (!fs.existsSync(joinPaths(dirPath, "index.d.ts"))) {
throw new Error(`${dirPath}: Must contain 'index.d.ts'.`);
}
return { errors };
}
Loading