completed-docs: Add support for enum members #2911
Conversation
src/rules/completedDocsRule.ts
Outdated
| // the requirements, use the node that is being checked. | ||
| if (requirementNode === undefined) { | ||
| requirementNode = node; | ||
| } |
There was a problem hiding this comment.
You could replace these lines with requirementNode: ts.Declaration = node
There was a problem hiding this comment.
Oh cool, I didn't realise you could do that!
| this.checkComments(node, this.describeNode(nodeType), comments, requirementNode); | ||
| } | ||
|
|
||
| private describeNode(nodeType: DocType): string { |
There was a problem hiding this comment.
Seems a little hardcody to just do this to ARGUMENT_ENUM_MEMBERS. How about generalizing the logic by taking in both node and requirementNode? If node is an ancestor of requirementNode, append " members" to the description.
There was a problem hiding this comment.
Yeah, I thought about some sort of camel-case-to-words thing, but it seemed like overkill. Now that I've changed enumMembers to enum-members, I could just do:
return nodeType.replace("-", " ");
| /** | ||
| * ... | ||
| */ | ||
| enum EnumWithMember { |
There was a problem hiding this comment.
Nit: EnumWithMembers
src/rules/completedDocsRule.ts
Outdated
| @@ -39,6 +39,7 @@ export const ALL = "all"; | |||
|
|
|||
| export const ARGUMENT_CLASSES = "classes"; | |||
| export const ARGUMENT_ENUMS = "enums"; | |||
| export const ARGUMENT_ENUM_MEMBERS = "enumMembers"; | |||
There was a problem hiding this comment.
Nit: other rules have options like "check-accessor", "member-variable-declaration", "array-simple", etc. This should probably be "enum-member".
There was a problem hiding this comment.
enum-member or enum-members?
There was a problem hiding this comment.
Ah yes, the plural :)
|
What's the convention for pushing changes to a pull request? Do I make a new commit and push it, or should I be amending my previous commit and force pushing? |
|
I've always just pushed additional and nobody's complained yet. |
|
FYI, I've just rebased these changes onto master to fix the merge conflict caused by #2924. |
|
thanks @reduckted! |
PR checklist
Overview of change:
I've extended the
completed-docsrule to check enum members. This is disabled by default and can be enabled by including a new argument "enum-members". This can also be configured to only require documentation for members of internal enums or exported enums.CHANGELOG.md entry:
[new-rule-option]
completed-docs: Addenum-membersoption