MetaMask / eslint-config

Shareable MetaMask ESLint config
MIT License
6 stars 16 forks source link

Verify that type aliases and interfaces, as well as members for these, have documentation #223

Open mcmire opened 2 years ago

mcmire commented 2 years ago

Currently if we have an interface like:

interface PackageMetadata {
  readonly dirName: string;
  readonly manifest: PackageManifest | MonorepoPackageManifest;
  readonly name: string;
  readonly dirPath: string;
}

or a type alias like:

type PackageMetadata = {
  readonly dirName: string;
  readonly manifest: PackageManifest | MonorepoPackageManifest;
  readonly name: string;
  readonly dirPath: string;
}

we are not required to supply documentation for these. This seems inconsistent with our rules. I think we should try to provide documentation for types as much as possible so that we can always look them up in our editors (and such documentation is useful when we publish tsdocs eventually).

We should be able to achieve this by configuring the require-jsdoc rule like so: https://github.com/gajus/eslint-plugin-jsdoc/issues/647

mcmire commented 2 years ago

While we are doing this, we should enforce the correct way to document types for TypeScript files. Although the JSDoc documentation suggests using @property, this tag is actually not supported by TypeDoc. Instead, they prefer you document each property on its own. An example of this can be seen here: https://github.com/TypeStrong/typedoc/blob/2e8af773a2e8f37af547e66bed07c01bf30cbb91/example/src/types.ts#L17

Mrtenz commented 2 years ago

Although the JSDoc documentation suggests using @property, this tag is actually not supported by TypeDoc. Instead, they prefer you document each property on its own.

How would we do this for inferred types, e.g., from superstruct? I've been doing this in a few places:


/**
 * @property bar - Baz.
 */
type Foo = Infer<typeof FooStruct>;
mcmire commented 2 years ago

@Mrtenz Hmm... That's a good point. @property would make more sense in that context.

I said that TypeDoc doesn't support @property, but that doesn't mean it doesn't render it. For context here is the difference between documenting properties in the docstring and documenting them individually. Given this code:

/**
 * ContactEntry representation
 *
 * @property address - Hex address of a recipient account
 * @property name - Nickname associated with this address
 * @property importTime - Data time when an account as created/imported
 */
export interface ContactEntry {
  /** Hello I am an address */
  address: string;
  name: string;
  importTime?: number;
}

TypeDoc produces:

Screenshot 2022-11-14 at 9 30 56 AM

As you can see, the box at the top includes the @property lines, and the docstrings for the properties themselves are listed in the Properties section.

And of course here's what VSCode shows when you hover over ContactEntry first and address second:

Screenshot 2022-11-14 at 9 39 58 AM Screenshot 2022-11-14 at 9 40 07 AM

Maybe using @property is not horrible?

mcmire commented 1 year ago

Revisiting this. It appears that TypeDoc and VSCode more or less do what we want here, but require some tweaks. So I think in order to truly support this we would want to:

Mrtenz commented 1 year ago

This seems to be the case for IntelliJ IDEA as well. Using @property works, but when using inline comments it doesn't show up. Ideally we support @property in TypeDoc at a minimum, and the other two points would be a "nice to have".

We could start by opening issues in those projects?

mcmire commented 1 year ago

@Mrtenz Yup, absolutely. I'll start by doing that.