feat(jsii-diff): add tool to check API compatibility by rix0rrr · Pull Request #415 · aws/jsii

rix0rrr · 2019-03-31T18:46:51Z

Add parsing semantics for various common capabilities to doc comments
(summary, remarks, stability and others).

Add a tool uses stability annotations in the doc comments to check
whether two JSII assemblies are API-compatible.

Fixes #358

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Add parsing semantics for various common capabilities to doc comments (summary, remarks, stability and others). Add a tool uses stability annotations in the doc comments to check whether two JSII assemblies are API-compatible.

- Rename `@see` field back to 'see'. - Treat unannotated APIs as `@stable` by default. - Add feature to leave out NPM package name by default for very concise comparison against latest release: `jsii-diff npm:`. - Split out struct vs ref interface comparisons to simplify. - Split out tests for same reason. - Take inherited constructors/methods into account. - Some more explanations in readme. - Fixed an oopsie, can't add required arguments to methods.

Merge remote-tracking branch 'origin/master' into huijbers/jsii-diff

rix0rrr · 2019-04-02T12:37:47Z

On the discussion of @subclassable:

I will lift it to a first-class annotation in the JSII spec. The idea being that JSII client generators will have the ability to look at this annotation and add sealed/final to classes, and potentially a @DoNotImplement annotation to interfaces which could be processed by an annotation processor.

On whether the annotation burden should be on @sealed or @subclassable:

Designing something for subclassing takes design effort. It makes sense therefore to consider classes/interfaces closed for subclassing by default, and only explicitly opening them up for subclassing by annotating them.
Consider what happens if you get it wrong? Opening up for subclassing == adhering to a stronger contract == a one-way door (you cannot opt out of subclassability without breaking compatibility). If the default is sealed and you figure out you got it wrong, you can make a class @subclassable without breaking backwards compat. If the default is open and you figure out you got it wrong, adding the @sealed annotation would require bumping the MV.

For both of these reasons, I think the default should be sealed and @subclassable should be the opt-in annotation.

RomainMuller · 2019-04-03T07:59:38Z

-        /// <summary>getXxx() is not allowed (see negatives), but getXxx(a, ...) is okay.</summary>
+        /// <remarks>
+        /// remarks: ..) is okay.
+        /// summary: getXxx() is not allowed (see negatives), but getXxx(a, .


…/jsii-diff

…s object instead of taking a mutable one

RomainMuller · 2019-04-03T11:21:01Z

  }
-  const module = await fs.readJSON(JSII_ASSEMBLY_FILE, { encoding: 'utf-8' }) as spec.Assembly;
+  const module = spec.validateAssembly(await fs.readJSON(JSII_ASSEMBLY_FILE, { encoding: 'utf-8' }));
  if (!module.name) { throw new Error(`Could not find package in ${JSII_ASSEMBLY_FILE}`); }


I don't reckon this is possible anymore, not that you've run through validateAssembly :)

RomainMuller

👌🏻 Dope. Would be nice to have follow-up issues created for the couple of low-impact items you're leaving out (colored output, this kind of things).

rix0rrr · 2019-04-03T11:26:42Z

Done. Don't like what the C# generator is doing to the doc comments on final glance, so I'm going to have to dive in there to finish this off.

eladb · 2019-04-03T11:22:40Z

+    }
+  }
+
+  if (original.docs.isSubclassable && !updated.docs.isSubclassable) {


eladb · 2019-04-03T11:24:16Z

+}
+
+export function compareStruct(original: reflect.InterfaceType, updated: reflect.InterfaceType, context: ComparisonContext) {
+  // We don't compare structs here; they will be evaluated for compatibility


looks weird. wouldn't it make sense to just have an argument that says if this is an input/output struct? [and again I am going back to the intuition that this is an inherent property of a struct]

eladb · 2019-04-03T11:28:22Z

+     * If present, this block indicates that an API item is no longer supported and may be
+     * removed in a future release.  The `@deprecated` tag must be followed by a sentence
+     * describing the recommended alternative.  Deprecation recursively applies to members
+     * of a container.  For example, if a class is deprecated, then so are all of its members.


Are we propagating this in the jsii manifest? (we should)

I've been going back and forth on this, and ultimately decided that the assembly would map most closely to the source of truth (the source code). The propagation analysis is done by jsii-reflect.

Sounds good

eladb · 2019-04-03T11:29:53Z

+     *
+     * @default false
+     */
+    subclassable?: boolean;


I am wondering if we should separate all these flags from "docs" into some some other struct associated with each type, but I guess pragmatically we can leave them here.

Was thinking the same, ended up with this for now.

Maybe also we can expose them as properties in jsii-reflect to improve discoverability...

I was originally doing that, but because of the type inheritance tree in jsii-reflect it turned into copy/pasting a bunch of similar properties around and then I decided to just put everything on Docs. But yeah.

feat(jsii-diff): add tool to check API compatibility

bf1b03f

Add parsing semantics for various common capabilities to doc comments (summary, remarks, stability and others). Add a tool uses stability annotations in the doc comments to check whether two JSII assemblies are API-compatible.

rix0rrr requested review from a team, costleya and dstufft as code owners March 31, 2019 18:46

eladb reviewed Apr 1, 2019

View reviewed changes

Rico Huijbers added 3 commits April 1, 2019 16:01

Rebuild

90034a2

Rebuild

a51658c

Merge remote-tracking branch 'origin/master' into huijbers/jsii-diff

Move @subclassable to spec and make it persistent in API contract

64cc8c2

RomainMuller suggested changes Apr 3, 2019

View reviewed changes

Rico Huijbers added 5 commits April 3, 2019 10:45

Review comments

ddd7ca1

Merge commit '8e0ba88021c6f7bebb9913af3928618a6099e92f' into huijbers…

ba059d8

…/jsii-diff

Reflow assembler.ts

c24d932

Merge remote-tracking branch 'origin/master' into huijbers/jsii-diff

e1d4124

Remove empty trailing lines from generated javadoc

4c827a1

This was referenced Apr 3, 2019

feat(jsii): Erase un-exported base interfaces instead of prohibiting those #426

Merged

feat(jsii): Erase un-exported base classes instead of prohibiting those #425

Merged

Use validateAssembly(), make compareAssemblies() return the Mismatche…

54d3f3e

…s object instead of taking a mutable one

eladb mentioned this pull request Apr 3, 2019

Enumerate the allowed doc properties and their meaning #358

Closed

RomainMuller reviewed Apr 3, 2019

View reviewed changes

RomainMuller approved these changes Apr 3, 2019

View reviewed changes

eladb approved these changes Apr 3, 2019

View reviewed changes

Rico Huijbers added 2 commits April 3, 2019 13:38

Review comments

d7f237e

Fix doc generation for .NET

11fa410

rix0rrr merged commit 9cfd867 into master Apr 3, 2019

rix0rrr deleted the huijbers/jsii-diff branch April 3, 2019 12:19

Conversation

rix0rrr commented Mar 31, 2019 • edited by RomainMuller Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rix0rrr commented Apr 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RomainMuller left a comment

Choose a reason for hiding this comment

Uh oh!

rix0rrr commented Apr 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rix0rrr commented Mar 31, 2019 •

edited by RomainMuller

Loading