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(jsii): check that referenced @param's exist #431

Merged
merged 2 commits into from
Apr 4, 2019

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Apr 4, 2019

Verify integrity of the documentation by forcing the parameter names
referred to in @param declarations to actually exist.

Fixes #422.

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

Verify integrity of the documentation by forcing the parameter names
referred to in @param declarations to actually exist.

Fixes #422.
@rix0rrr rix0rrr requested a review from a team as a code owner April 4, 2019 10:14
@@ -645,18 +645,10 @@ export class Assembler implements Emitter {
}

/**
* Register documentations on a ``spec.Documentable`` entry.
*
* @param sym the symbol holding the JSDoc information
Copy link
Contributor

Choose a reason for hiding this comment

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

It is sad that you're dropping rich documentation and making it less nice, while you're fixing the problems with it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't feel like it was adding a lot of value. If the parameter is declared as sym: Symbol and the documentation basically says "this is the symbol", then I don't feel like the documentation is adding a lot of information.

Similarly for the return value. Declaration says: function(...): Docs, I don't feel the need to add @returns the docs.

const actualNames = new Set((method.parameters || []).map(p => p.name));
for (const param of params) {
if (!actualNames.has(param)) {
this._diagnostic(methodSym.valueDeclaration, ts.DiagnosticCategory.Error,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a warning instead? I mean - is there any reason to actually fail on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could, but I'm wary. In my experience, warnings don't get fixed. But sure, I'll throw in a warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can turn this into an error in a later version if you like (using. warning first gives some notice period, which is great for CDK releases :D)

Copy link
Contributor

Choose a reason for hiding this comment

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

And - feel free to record this in an issue so we don't forget!

@rix0rrr rix0rrr merged commit 265c304 into master Apr 4, 2019
@rix0rrr rix0rrr deleted the huijbers/params-must-exist branch April 4, 2019 11:37
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.

2 participants