-
Notifications
You must be signed in to change notification settings - Fork 246
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ import log4js = require('log4js'); | |
import path = require('path'); | ||
import ts = require('typescript'); | ||
import { JSII_DIAGNOSTICS_CODE } from './compiler'; | ||
import { parseSymbolDocumentation } from './docs'; | ||
import { getReferencedDocParams, parseSymbolDocumentation } from './docs'; | ||
import { Diagnostic, Emitter } from './emitter'; | ||
import literate = require('./literate'); | ||
import { ProjectInfo } from './project-info'; | ||
|
@@ -645,18 +645,10 @@ export class Assembler implements Emitter { | |
} | ||
|
||
/** | ||
* Register documentations on a ``spec.Documentable`` entry. | ||
* | ||
* @param sym the symbol holding the JSDoc information | ||
* @param documentable the entity being documented | ||
* | ||
* @returns ``documentable`` | ||
* Return docs for a symbol | ||
*/ | ||
private _visitDocumentation(sym: ts.Symbol): spec.Docs | undefined { | ||
const comment = ts.displayPartsToString(sym.getDocumentationComment(this._typeChecker)).trim(); | ||
|
||
// Right here we'll just guess that the first declaration site is the most important one. | ||
const result = parseSymbolDocumentation(comment, sym.getJsDocTags()); | ||
const result = parseSymbolDocumentation(sym, this._typeChecker); | ||
|
||
for (const diag of result.diagnostics || []) { | ||
this._diagnostic(sym.declarations[0], | ||
|
@@ -669,6 +661,20 @@ export class Assembler implements Emitter { | |
return !allUndefined ? result.docs : undefined; | ||
} | ||
|
||
/** | ||
* Check that all parameters the doc block refers to with a @param declaration actually exist | ||
*/ | ||
private _validateReferencedDocParams(method: spec.Method, methodSym: ts.Symbol) { | ||
const params = getReferencedDocParams(methodSym); | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||
`In doc block of '${method.name}', '@param ${param}' refers to a nonexistent parameter.`); | ||
} | ||
} | ||
} | ||
|
||
private async _visitInterface(type: ts.Type, namespace: string[]): Promise<spec.InterfaceType | undefined> { | ||
if (LOG.isTraceEnabled()) { | ||
LOG.trace(`Processing interface: ${colors.gray(namespace.join('.'))}.${colors.cyan(type.symbol.name)}`); | ||
|
@@ -846,6 +852,8 @@ export class Assembler implements Emitter { | |
}); | ||
} | ||
|
||
this._validateReferencedDocParams(method, symbol); | ||
|
||
type.methods = type.methods || []; | ||
if (type.methods.find(m => m.name === method.name && m.static === method.static) != null) { | ||
LOG.trace(`Dropping re-declaration of ${colors.green(type.fqn)}#${colors.cyan(method.name!)}`); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
///!MATCH_ERROR: In doc block of 'helpMe', '@param benKenobi' refers to a nonexistent parameter. | ||
|
||
export class Foo { | ||
/** | ||
* Ask for help | ||
* | ||
* @param benKenobi The person you want to ask for help | ||
*/ | ||
public helpMe(obiWanKenobi: string) { | ||
Array.isArray(obiWanKenobi); | ||
} | ||
} |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
.