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

Improvement jsdoc/require-returns rule #518

Closed
alejandroclaro opened this issue Apr 16, 2020 · 4 comments
Closed

Improvement jsdoc/require-returns rule #518

alejandroclaro opened this issue Apr 16, 2020 · 4 comments

Comments

@alejandroclaro
Copy link

alejandroclaro commented Apr 16, 2020

The current jsdoc/require-returns implementation depends on the presence of a return statement, but an inconsistent case remains even when forceReturnsWithAsync is set to true.

It will be nice, at least in Typescript, to detect when async/await is used and the return is Promise<void> type. It strange and not very insightful document this case just because the type change from void to Promise. For example:

/**
 * Do something.
 *
 * @throws {Error} If something bad occurs.
 */
function doFoo(): void {
  // Do something.
}

/**
 * Do something.
 *
 * @returns The promise that is resolved once something is completed.
 *
 * @throws {Error} If something bad occurs.
 */
async function doBar(): Promise<void> {
  // Do something.
}

/**
 * Do something.
 *
 * @returns The promise that is resolved once something is completed.
 *
 * @throws {Error} If something bad occurs.
 */
async function doBaz(): Promise<void> {
  return new Promise<void>((resolve, reject) => {
    // For some reason its more convenient at some point to return a contruction like this.
  });
}

doFoo();
await doBar();
await doBaz();

To me all methods are similar in structure and documentation. However, the rule makes mandatory to document the result in doBar() and/or doBaz().

It would be nice if you can configure the rule to avoid having to document Promise<void> return when using async / wait.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 16, 2020

forceReturnsWithAsync should be set if you do want to force there to be a return with async functions when undefined is the return value. Otherwise, you don't have to document returns for such cases. If you browse through the examples, you should see this.

Closing as I think that should address, but feel free to comment further and clarify if I have misunderstood.

@brettz9 brettz9 closed this as completed Apr 16, 2020
@alejandroclaro
Copy link
Author

@brettz9 I think that may I didn't explain the issue well.

What I'm trying to explain is that we have both cases:

/**
 * Do something.
 *
 * @throws {Error} If something bad occurs.
 */
async function doBar(): Promise<void> {
  // Do something.
}

/**
 * Do something.
 *
 * @returns The promise that is resolved once something is completed.
 *
 * @throws {Error} If something bad occurs.
 */
async function doBaz(): Promise<void> {
  return new Promise<void>((resolve, reject) => {
    // For some reason its more convenient at some point to return a contruction like this.
  });
}

Sometimes it's convenient to write the function like in doBaz(). When we do this, jdoc/require-return
emit s warning because the return documentation is missing. However, we see no value in documenting Promise<void> returns in typescript. I think this should be equivalent to doBar().

To make it consistent, we turn on forceReturnsWithAsync, so we have to document the return value every way when Promise<void> is used. But what we want is the other way. We want to have the possibility to no document the return when Promise<void> in typescript, because most of the time it does not make sense. The return type information is in the signature of the function, and void usually means nothing.

@brettz9 brettz9 reopened this May 18, 2020
@brettz9 brettz9 added bug and removed question labels May 18, 2020
@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2020

There may be some cases where a return ultimately resolves only to undefined, but as we don't auto-detect such possibilities, indeed, I think it is pretty clear that the expectation should be changed here to be safe. Even i it returns undefined, those looking at the function will appreciate knowing what it resolves to.

@gajus
Copy link
Owner

gajus commented May 18, 2020

🎉 This issue has been resolved in version 25.4.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants