Tslint: completed-docs should not require documentation on merged namespaces

Created on 11 Mar 2019  ยท  8Comments  ยท  Source: palantir/tslint

Bug Report

  • __TSLint version__: 5.13.1
  • __TypeScript version__: 3.3.3333
  • __Running TSLint via__: CLI

TypeScript code being linted

/**
 * A sample class
 */
export class Foo {
    // ...whatever
}

export namespace Foo {
    /**
     * An array of Foo
     */
    export type Bar = Array<Foo>;
}

with tslint.json configuration:

{
    "rules": {
        "completed-docs": [
            true, {
                "namespaces": {
                    "visibilities": ["exported"]
                }
            }
        ]
    }
}

Actual behavior

completed-docs fails on the Foo namespace:

Screen Shot 2019-03-11 at 4 09 40 PM

Expected behavior

The namespace Foo in this example is merged with class Foo, so the namespace should not require its own documentation. The namespace is essentially an extension of the class, so the documentation on the class itself is most relevant and can be used by IDEs (example from IntelliJ IDEA):

Screen Shot 2019-03-11 at 4 10 41 PM

Note that namespaces can also be merged with functions, enums, etc.. If a namespace is merged with anything, then it shouldn't require its own documentation.

Requires Type Checker Bug ๐ŸŒน R.I.P. ๐ŸŒน

Most helpful comment

I am seeing this same error on non-merged namespaces after upgrading from TSLint 5.11.0 to 5.16.0 with TypeScript 3.4.4, running TSLint via CLI:

/**
 * Useful accessors and functionality for APIUsers.
 */
export namespace APIUserModel {
    /**
     * Returns whether the specified user is an employee.
     *
     * @param user The user to check.
     */
    export function isEmployee(user: { roles: Array<string> }): boolean {
        return user.roles.indexOf('EMPLOYEE') >= 0;
    }
}

tslint.json:

{
    "rules": {
        "completed-docs": [true, {
            "functions": {"visibilities": ["exported"]}
        }]
    }
}

Actual behavior:

>> ERROR: api-user-model.ts:10:5 - Documentation must exist for exported functions.

Expected behavior: no errors; the exported function has documentation.

All 8 comments

I'm not sure about this... IMO, the unintuitive nature of namespace merging (it's not a JS idiom) often warrants its own documentation, especially if you're working in a strict codebase which has completed-docs enabled... I'd be inclined to leave the rule as-is. You might as well explain in a comment why you are using essentially a deprecated feature of TS.

Wrapping the contents of an entire module in a namespace is a deprecated pattern, but namespaces as a whole are not deprecated. There are other valid use cases for namespaces for organizing types related to classes/functions.

My sample code is an example of using a namespace to organize "inner" type definitions relevant to a particular class.

Another pattern is a function with a single "params" parameter defined as an interface, that is only relevant to that specific function:

/**
  * Blah blah.
  * @param params - Params object.
  */
export function foo(params: foo.Params) {
    // ...
}

export namespace foo {
    /**
     * Params structure for {@link foo}.
     */
    export interface Params {
        /**
         * Blah blah
         */
        bar?: string;
        /**
         * Blah blah
         */
        baz?: number;
    }
}

The documentation for such a namespace would always be the same thing: "Stuff related to function foo", or "Stuff related to class Foo".

When using namespace merging, you are simply "scoping" its contents within the name of the class/function/enum that you merged it with. There's nothing additional to document about the purpose of the namespace.

I am seeing this same error on non-merged namespaces after upgrading from TSLint 5.11.0 to 5.16.0 with TypeScript 3.4.4, running TSLint via CLI:

/**
 * Useful accessors and functionality for APIUsers.
 */
export namespace APIUserModel {
    /**
     * Returns whether the specified user is an employee.
     *
     * @param user The user to check.
     */
    export function isEmployee(user: { roles: Array<string> }): boolean {
        return user.roles.indexOf('EMPLOYEE') >= 0;
    }
}

tslint.json:

{
    "rules": {
        "completed-docs": [true, {
            "functions": {"visibilities": ["exported"]}
        }]
    }
}

Actual behavior:

>> ERROR: api-user-model.ts:10:5 - Documentation must exist for exported functions.

Expected behavior: no errors; the exported function has documentation.

๐Ÿ’€ _It's time!_ ๐Ÿ’€

TSLint is deprecated and no longer accepting pull requests other than security fixes. See #4534. โ˜ ๏ธ
We recommend you instead use typescript-eslint to lint your TypeScript code with ESLint. โœ…

๐Ÿ‘‹ It was a pleasure open sourcing with you!

I am seeing this same error on non-merged namespaces after upgrading from TSLint 5.11.0 to 5.16.0 with TypeScript 3.4.4, running TSLint via CLI:

/**
 * Useful accessors and functionality for APIUsers.
 */
export namespace APIUserModel {
    /**
     * Returns whether the specified user is an employee.
     *
     * @param user The user to check.
     */
    export function isEmployee(user: { roles: Array<string> }): boolean {
        return user.roles.indexOf('EMPLOYEE') >= 0;
    }
}

tslint.json:

{
    "rules": {
        "completed-docs": [true, {
            "functions": {"visibilities": ["exported"]}
        }]
    }
}

Actual behavior:

>> ERROR: api-user-model.ts:10:5 - Documentation must exist for exported functions.

Expected behavior: no errors; the exported function has documentation.

Is there any workaround to get this fixed?

๐Ÿค– Beep boop! ๐Ÿ‘‰ TSLint is deprecated ๐Ÿ‘ˆ _(#4534)_ and you should switch to typescript-eslint! ๐Ÿค–

๐Ÿ”’ This issue is being locked to prevent further unnecessary discussions. Thank you! ๐Ÿ‘‹

Was this page helpful?
0 / 5 - 0 ratings