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 support for "external" stability #596

Merged
merged 5 commits into from
Jul 16, 2019
Merged
Show file tree
Hide file tree
Changes from 3 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
14 changes: 9 additions & 5 deletions packages/jsii-diff/lib/stability.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,21 @@ export function compareStabilities(original: reflect.Documentable & ApiElement,

function allowedTransitions(start: spec.Stability): spec.Stability[] {
switch (start) {
// Experimental can go to stable or be deprecated
// Experimental can go to stable, external, or be deprecated
case spec.Stability.Experimental:
return [spec.Stability.Stable, spec.Stability.Deprecated];
return [spec.Stability.Stable, spec.Stability.Deprecated, spec.Stability.External];

// Stable can be deprecated
// Stable can be deprecated, or switched to external
case spec.Stability.Stable:
return [spec.Stability.Deprecated];
return [spec.Stability.Deprecated, spec.Stability.External];

// Deprecated can be reinstated
case spec.Stability.Deprecated:
return [spec.Stability.Stable];
return [spec.Stability.Stable, spec.Stability.External];

// external can be stableified, or deprecated
case spec.Stability.External:
return [spec.Stability.Stable, spec.Stability.Deprecated];
}

throw new Error(`Unrecognized stability: ${start}`);
Expand Down
36 changes: 36 additions & 0 deletions packages/jsii-diff/test/test.diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,24 @@ export = {
test.done();
},

// ----------------------------------------------------------------------
async 'external stability violations are reported as warnings'(test: Test) {
const mms = await compare(`
/** @stability external */
export class Foo1 { }
`, `
export class Foo2 { }
`);

const experimentalErrors = false;
const diags = classifyDiagnostics(mms, experimentalErrors, new Set());

test.equals(1, diags.length);
test.equals(false, hasErrors(diags));

test.done();
},

// ----------------------------------------------------------------------
async 'warnings can be turned into errors'(test: Test) {
const mms = await compare(`
Expand All @@ -39,6 +57,24 @@ export = {
test.done();
},

// ----------------------------------------------------------------------
async 'external stability violations are never turned into errors'(test: Test) {
const mms = await compare(`
/** @stability external */
export class Foo1 { }
`, `
export class Foo2 { }
`);

const experimentalErrors = true;
const diags = classifyDiagnostics(mms, experimentalErrors, new Set());

test.equals(1, diags.length);
test.equals(false, hasErrors(diags));

test.done();
},

// ----------------------------------------------------------------------
async 'errors can be skipped'(test: Test) {
const mms = await compare(`
Expand Down
2 changes: 2 additions & 0 deletions packages/jsii-pacmak/lib/targets/java.ts
Original file line number Diff line number Diff line change
Expand Up @@ -771,6 +771,8 @@ class JavaGenerator extends Generator {
return 'Deprecated';
case spec.Stability.Experimental:
return 'Experimental';
case spec.Stability.External:
return 'External';
case spec.Stability.Stable:
return 'Stable';
}
Expand Down
3 changes: 2 additions & 1 deletion packages/jsii-reflect/lib/docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ export class Docs {
const stabilityPrecedence = {
[Stability.Deprecated]: 0,
[Stability.Experimental]: 1,
[Stability.Stable]: 2,
[Stability.External]: 2,
[Stability.Stable]: 3,
};

function lowestStability(a?: Stability, b?: Stability): Stability | undefined {
Expand Down
24 changes: 24 additions & 0 deletions packages/jsii-reflect/test/typesystem.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,30 @@ describe('Stability', () => {
expect(initializer.docs.stability).toEqual(Stability.Experimental);
expect(method.docs.stability).toEqual(Stability.Experimental);
});

test('external stability', async () => {
const ts = await typeSystemFromSource(`
/**
* @stability external
*/
export class Foo {
public foo() {
Array.isArray(3);
}
}

/**
* @stable
*/
export class SubFoo extends Foo {
}
`);

const classType = ts.findClass('testpkg.SubFoo');
const method = classType.allMethods.find(m => m.name === 'foo')!;

expect(method.docs.stability).toEqual(Stability.External);
});
});
});

Expand Down
6 changes: 6 additions & 0 deletions packages/jsii-spec/lib/spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,12 @@ export enum Stability {
* in breaking ways in a subsequent minor or patch version.
*/
Stable = 'stable',

/**
* This API is an representation of an API managed elsewhere and follows
* the other API's versioning model.
*/
External = 'external',
}

/**
Expand Down
57 changes: 39 additions & 18 deletions packages/jsii/lib/docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,24 @@ function parseDocParts(comments: string | undefined, tags: ts.JSDocTagInfo[]): D
docs.see = eatTag('see');
docs.subclassable = eatTag('subclassable') !== undefined ? true : undefined;

docs.stability = parseStability(eatTag('stability'), diagnostics);
// @experimental is a shorthand for '@stability experimental', same for '@stable'
const experimental = eatTag('experimental') !== undefined;
const stable = eatTag('stable') !== undefined;
// Can't combine them
if (countBools(docs.stability !== undefined, experimental, stable) > 1) {
diagnostics.push(`Use only one of @stability, @experimental or @stable`);
}
if (experimental) { docs.stability = spec.Stability.Experimental; }
if (stable) { docs.stability = spec.Stability.Stable; }

// Can combine '@stability deprecated' with '@deprecated <reason>'
if (docs.deprecated !== undefined) {
if (docs.stability !== undefined && docs.stability !== spec.Stability.Deprecated) {
diagnostics.push(`@deprecated tag requires '@stability deprecated' or no @stability at all.`);
}
docs.stability = spec.Stability.Deprecated;
}

if (docs.example && docs.example.indexOf('```') >= 0) {
// This is currently what the JSDoc standard expects, and VSCode highlights it in
Expand All @@ -97,26 +113,10 @@ function parseDocParts(comments: string | undefined, tags: ts.JSDocTagInfo[]): D
diagnostics.push('@example must be code only, no code block fences allowed.');
}

if (experimental && stable) {
diagnostics.push('Element is marked both @experimental and @stable.');
}

if (docs.deprecated !== undefined) {
if (docs.deprecated.trim() === '') {
diagnostics.push('@deprecated tag needs a reason and/or suggested alternatives.');
}
if (stable) {
diagnostics.push('Element is marked both @deprecated and @stable.');
}
if (experimental) {
diagnostics.push('Element is marked both @deprecated and @experimental.');
}
if (docs.deprecated !== undefined && docs.deprecated.trim() === '') {
diagnostics.push('@deprecated tag needs a reason and/or suggested alternatives.');
}

if (experimental) { docs.stability = spec.Stability.Experimental; }
if (stable) { docs.stability = spec.Stability.Stable; }
if (docs.deprecated) { docs.stability = spec.Stability.Deprecated; }

if (tagNames.size > 0) {
docs.custom = {};
for (const [key, value] of tagNames.entries()) {
Expand Down Expand Up @@ -184,3 +184,24 @@ function summaryLine(str: string) {
const PUNCTUATION = ['!', '?', '.', ';'].map(s => '\\' + s).join('');
const ENDS_WITH_PUNCTUATION_REGEX = new RegExp(`[${PUNCTUATION}]$`);
const FIRST_SENTENCE_REGEX = new RegExp(`^([^${PUNCTUATION}]+[${PUNCTUATION}] )`); // literal space at the end

function intBool(x: boolean): number {
return x ? 1 : 0;
}

function countBools(...x: boolean[]) {
return x.map(intBool).reduce((a, b) => a + b, 0);
}

function parseStability(s: string | undefined, diagnostics: string[]): spec.Stability | undefined {
if (s === undefined) { return undefined; }

switch (s) {
case 'stable': return spec.Stability.Stable;
case 'experimental': return spec.Stability.Experimental;
case 'external': return spec.Stability.External;
case 'deprecated': return spec.Stability.Deprecated;
}
diagnostics.push(`Unrecognized @stability: '${s}'`);
return undefined;
}
22 changes: 22 additions & 0 deletions packages/jsii/test/test.docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,28 @@ export = {
test.done();
},

// ----------------------------------------------------------------------

async 'can mark external'(test: Test) {
const assembly = await compile(`
/**
* @stability external
*/
export class Foo {
public floop() {
Array.isArray(3);
}
}
`);

const classType = assembly.types!['testpkg.Foo'] as spec.ClassType;
const method = classType.methods!.find(m => m.name === 'floop');

test.deepEqual(classType.docs!.stability, spec.Stability.External);
test.deepEqual(method!.docs!.stability, spec.Stability.External);
test.done();
Copy link
Contributor

Choose a reason for hiding this comment

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

Don’t we need a test which verifies that imported can break with only warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

},

// ----------------------------------------------------------------------
async 'can mark subclassable'(test: Test) {
const assembly = await compile(`
Expand Down