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

feat: Add full warnings list, dont ouput warning if disabled #1838

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
41 changes: 27 additions & 14 deletions src/assembler.ts
Original file line number Diff line number Diff line change
@@ -143,7 +143,9 @@ export class Assembler implements Emitter {
}
const readme = _loadReadme.call(this);
if (readme == null) {
this._diagnostics.push(JsiiDiagnostic.JSII_0003_MISSING_README.createDetached());
if (enabledWarnings['metadata/missing-readme']) {
this._diagnostics.push(JsiiDiagnostic.JSII_0003_MISSING_README.createDetached());
}
}
const docs = _loadDocs.call(this);

@@ -370,7 +372,10 @@ export class Assembler implements Emitter {
// since we are exposing a type of this assembly in this module's public API,
// we expect it to appear as a peer dependency instead of a normal dependency.
if (assembly) {
if (!(assembly.name in this.projectInfo.peerDependencies)) {
if (
!(assembly.name in this.projectInfo.peerDependencies) &&
enabledWarnings['metadata/missing-peer-dependency']
) {
this._diagnostics.push(
JsiiDiagnostic.JSII_0005_MISSING_PEER_DEPENDENCY.create(
referencingNode!, // Cheating here for now, until the referencingNode can be made required
@@ -1637,7 +1642,7 @@ export class Assembler implements Emitter {
const params = getReferencedDocParams(methodSym);
const actualNames = new Set((method.parameters ?? []).map((p) => p.name));
for (const param of params) {
if (!actualNames.has(param)) {
if (!actualNames.has(param) && enabledWarnings['documentation/non-existent-parameter']) {
this._diagnostics.push(
JsiiDiagnostic.JSII_7000_NON_EXISTENT_PARAMETER.create(
methodSym.valueDeclaration ?? methodSym.declarations?.[0],
@@ -1871,7 +1876,10 @@ export class Assembler implements Emitter {
return;
}

if (type.name === Case.pascal(symbol.name)) {
if (
type.name === Case.pascal(symbol.name) &&
enabledWarnings['language-compatibility/member-name-conflicts-with-type-name']
) {
this._diagnostics.push(
JsiiDiagnostic.JSII_5019_MEMBER_TYPE_NAME_CONFLICT.create(
declaration.name,
@@ -1952,15 +1960,17 @@ export class Assembler implements Emitter {
}

private _warnAboutReservedWords(symbol: ts.Symbol) {
if (!enabledWarnings['reserved-word']) {
return;
}

const reservingLanguages = isReservedName(symbol.name);
if (reservingLanguages) {
this._diagnostics.push(
JsiiDiagnostic.JSII_5018_RESERVED_WORD.create(_nameOrDeclarationNode(symbol), symbol.name, reservingLanguages),
);
if (enabledWarnings['language-compatibility/reserved-word']) {
const reservingLanguages = isReservedName(symbol.name);
if (reservingLanguages) {
this._diagnostics.push(
JsiiDiagnostic.JSII_5018_RESERVED_WORD.create(
_nameOrDeclarationNode(symbol),
symbol.name,
reservingLanguages,
),
);
}
}
}

@@ -1990,7 +2000,10 @@ export class Assembler implements Emitter {
| ts.AccessorDeclaration
| ts.ParameterPropertyDeclaration;

if (type.name === Case.pascal(symbol.name)) {
if (
type.name === Case.pascal(symbol.name) &&
enabledWarnings['language-compatibility/member-name-conflicts-with-type-name']
) {
this._diagnostics.push(
JsiiDiagnostic.JSII_5019_MEMBER_TYPE_NAME_CONFLICT.create(
signature.name,
6 changes: 5 additions & 1 deletion src/compiler.ts
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@ import { BASE_COMPILER_OPTIONS, convertForJson } from './tsconfig/compiler-optio
import { TypeScriptConfigValidator } from './tsconfig/tsconfig-validator';
import { ValidationError } from './tsconfig/validator';
import * as utils from './utils';
import { enabledWarnings } from './warnings';

const LOG = log4js.getLogger('jsii/compiler');
export const DIAGNOSTICS = 'diagnostics';
@@ -182,7 +183,10 @@ export class Compiler implements Emitter {

// emit a warning if validation is disabled
const rules = this.options.validateTypeScriptConfig ?? TypeScriptConfigValidationRuleSet.NONE;
if (rules === TypeScriptConfigValidationRuleSet.NONE) {
if (
rules === TypeScriptConfigValidationRuleSet.NONE &&
enabledWarnings['typescript-config/disabled-tsconfig-validation']
) {
utils.logDiagnostic(
JsiiDiagnostic.JSII_4009_DISABLED_TSCONFIG_VALIDATION.create(undefined, this.configPath),
this.projectRoot,
7 changes: 5 additions & 2 deletions src/directives.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as ts from 'typescript';
import { JsiiDiagnostic } from './jsii-diagnostic';
import { enabledWarnings } from './warnings';

/**
* TSDoc-style directives that can be attached to a symbol.
@@ -40,7 +41,7 @@ export class Directives {
break;
case 'jsii':
const comments = getComments(tag);
if (comments.length === 0) {
if (comments.length === 0 && enabledWarnings['jsii-directive/missing-argument']) {
onDiagnostic(JsiiDiagnostic.JSII_2000_MISSING_DIRECTIVE_ARGUMENT.create(tag));
continue;
}
@@ -50,7 +51,9 @@ export class Directives {
this.ignore ??= jsdocNode;
break;
default:
onDiagnostic(JsiiDiagnostic.JSII_2999_UNKNOWN_DIRECTIVE.create(jsdocNode, text));
if (enabledWarnings['jsii-directive/unknown']) {
onDiagnostic(JsiiDiagnostic.JSII_2999_UNKNOWN_DIRECTIVE.create(jsdocNode, text));
}
break;
}
}
17 changes: 3 additions & 14 deletions src/main.ts
Original file line number Diff line number Diff line change
@@ -13,9 +13,7 @@ import { emitSupportPolicyInformation } from './support';
import { TypeScriptConfigValidationRuleSet } from './tsconfig';
import * as utils from './utils';
import { VERSION } from './version';
import { enabledWarnings } from './warnings';

const warningTypes = Object.keys(enabledWarnings);
import { enabledWarnings, silenceWarnings } from './warnings';

function choiceWithDesc(
choices: { [choice: string]: string },
@@ -91,7 +89,7 @@ const ruleSets: {
group: OPTION_GROUP.JSII,
type: 'array',
default: [],
desc: `List of warnings to silence (warnings: ${warningTypes.join(',')})`,
desc: `List of warnings to silence (warnings: ${Object.keys(enabledWarnings).join(',')})`,
})
.option('strip-deprecated', {
group: OPTION_GROUP.JSII,
@@ -149,16 +147,7 @@ const ruleSets: {

const { projectInfo, diagnostics: projectInfoDiagnostics } = loadProjectInfo(projectRoot);

// disable all silenced warnings
for (const key of argv['silence-warnings']) {
if (!(key in enabledWarnings)) {
throw new utils.JsiiError(
`Unknown warning type ${key as any}. Must be one of: ${warningTypes.join(', ')}`,
);
}

enabledWarnings[key] = false;
}
silenceWarnings(argv['silence-warnings'] as string[]);

configureCategories(projectInfo.diagnostics ?? {});

23 changes: 13 additions & 10 deletions src/project-info.ts
Original file line number Diff line number Diff line change
@@ -10,6 +10,7 @@ import { findDependencyDirectory } from './common/find-utils';
import { JsiiDiagnostic } from './jsii-diagnostic';
import { TypeScriptConfigValidationRuleSet } from './tsconfig';
import { JsiiError, parsePerson, parseRepository } from './utils';
import { enabledWarnings } from './warnings';

// eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports
const spdx: Set<string> = require('spdx-license-list/simple');
@@ -178,16 +179,18 @@ export function loadProjectInfo(projectRoot: string): ProjectInfoResult {
const range = new semver.Range(_resolveVersion(rng as string, projectRoot).version);
const minVersion = semver.minVersion(range)?.raw;

if (!(name in devDependencies) || devDependencies[name] !== `${minVersion}`) {
diagnostics.push(
JsiiDiagnostic.JSII_0006_MISSING_DEV_DEPENDENCY.createDetached(
name,
`${rng as any}`,
`${minVersion}`,
`${devDependencies[name]}`,
),
);
continue;
if (enabledWarnings['metadata/missing-dev-dependency']) {
if (!(name in devDependencies) || devDependencies[name] !== `${minVersion}`) {
diagnostics.push(
JsiiDiagnostic.JSII_0006_MISSING_DEV_DEPENDENCY.createDetached(
name,
`${rng as any}`,
`${minVersion}`,
`${devDependencies[name]}`,
),
);
continue;
}
}
}

36 changes: 34 additions & 2 deletions src/warnings.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,39 @@
import { JsiiError } from './utils';

/**
* Indicates which warnings are currently enabled. By default all warnings are
* enabled, and can be silenced through the --silence-warning option.
*/
export const enabledWarnings: { [name: string]: boolean } = {
'reserved-word': true,
export const enabledWarnings = {
'metadata/missing-readme': true,
'metadata/missing-peer-dependency': true,
'metadata/missing-dev-dependency': true,
'jsii-directive/missing-argument': true,
'jsii-directive/struct-on-non-interface': true,
'jsii-directive/unknown': true,
'typescript-config/disabled-tsconfig-validation': true,
'language-compatibility/reserved-word': true,
'language-compatibility/member-name-conflicts-with-type-name': true,
'documentation/non-existent-parameter': true,
};

export const silenceWarnings = (warnings: string[]): void => {
const legacyWarningKeyReplacement: { [key: string]: string } = {
'reserved-word': 'language-compatibility/reserved-word',
};
const legacyWarningKeys = Object.keys(legacyWarningKeyReplacement);

for (const key of warnings) {
if (!(key in enabledWarnings) && !legacyWarningKeys.includes(key)) {
throw new JsiiError(
`Unknown warning type ${key as any}. Must be one of: ${Object.keys(enabledWarnings).join(', ')}`,
);
}

if (legacyWarningKeys.includes(key)) {
enabledWarnings[legacyWarningKeyReplacement[key] as keyof typeof enabledWarnings] = false;
} else {
enabledWarnings[key as keyof typeof enabledWarnings] = false;
}
}
};
36 changes: 36 additions & 0 deletions test/warnings.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { DiagnosticCategory } from 'typescript';
import { Code, JsiiDiagnostic } from '../src/jsii-diagnostic';
import { JsiiError } from '../src/utils';
import { enabledWarnings, silenceWarnings } from '../src/warnings';

describe('enabledWarnings', () => {
test('contains all available Jsii warnings', () => {
const definedWarnings = Object.keys(JsiiDiagnostic).reduce((warnings, warningKey) => {
const code = JsiiDiagnostic[warningKey as keyof typeof JsiiDiagnostic];
if (code instanceof Code && code.category === DiagnosticCategory.Warning) {
warnings[code.name] = true;
}
return warnings;
}, {} as { [name: string]: boolean });

expect(enabledWarnings).toStrictEqual(definedWarnings);
});
});

describe('silenceWarnings', () => {
test('sets enabledWarnings key to false', () => {
expect(enabledWarnings['metadata/missing-readme']).toBe(true);
silenceWarnings(['metadata/missing-readme']);
expect(enabledWarnings['metadata/missing-readme']).toBe(false);
});

test('translates legacy key to current Code.name', () => {
expect(enabledWarnings['language-compatibility/reserved-word']).toBe(true);
silenceWarnings(['reserved-word']);
expect(enabledWarnings['language-compatibility/reserved-word']).toBe(false);
});

test('throws when key is not valid', () => {
expect(() => silenceWarnings(['invalid-warning'])).toThrow(JsiiError);
});
});