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

5.0.0-alpha.3: valid-jsdoc unexpectedly expects returns for async functions, even with requireReturn: false #10386

Closed
Standard8 opened this issue May 21, 2018 · 4 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@Standard8
Copy link
Contributor

Standard8 commented May 21, 2018

Tell us about your environment

  • ESLint Version: 5.0.0-alpha.3
  • Node Version: 8.11.2
  • npm Version: 5.6.0

What parser (default, Babel-ESLint, etc.) are you using? default

Please show your full configuration:

Configuration
module.exports = {
  "rules": {
  },
  "env": {
    "es6": true
  },
  "parserOptions": {
    "ecmaVersion": 9,
  },
  "rules": {
    "valid-jsdoc": ["error", {
      "prefer": {
	"return": "returns",
      },
      "requireReturn": false,
      "requireReturnDescription": false,
    }]
  }
};

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

/**                                                                                                                                                       
 * A comment.                                                                                                                                             
 */
async function foo() {
  if (!this.bar) {
    return;
  }

  await this.bar();
}
./node_modules/.bin/eslint eslint5.0test.js

What did you expect to happen?

No failures

What actually happened? Please include the actual, raw output from ESLint.

eslint4.0test.js
  1:1  error  Missing JSDoc @returns for function  valid-jsdoc

✖ 1 problem (1 error, 0 warnings)

This is a possible regression from #10161

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label May 21, 2018
@aladdin-add aladdin-add added bug ESLint is working incorrectly rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 21, 2018
@Standard8
Copy link
Contributor Author

Now I'm having another look at #10161, I'm seeing that since that has been implemented, async functions must always have a @returns statement in the jsdoc, regardless of the requireReturn value.

For a function that is async the fact it returns a promise is implicit in its definition. Therefore, documentation wise, it could be treated the same as a function that returns nothing.

Unfortunately, I'm not quite seeing how making this optional could fit into the current configuration.

So ideas here would be appreciated, /cc @not-an-aardvark @rachx

@rachx
Copy link
Contributor

rachx commented Jun 14, 2018

For a function that is async the fact it returns a promise is implicit in its definition. Therefore, documentation wise, it could be treated the same as a function that returns nothing.

In the original issue, some users suggested that with requireReturn: false, it should also allow them to add jsdoc descriptions as async functions returns a promise.

To make it optional for async function just like constructors and support both preferences, how about check if it is not an async function before reporting missing jsdocs?

// check for functions missing @returns
if (!isOverride && !hasReturns && !hasConstructor && !isInterface &&
node.parent.kind !== "get" && node.parent.kind !== "constructor" &&
node.parent.kind !== "set" && !isTypeClass(node)) {
if (requireReturn || functionData.returnPresent) {
context.report({
node: jsdocNode,
message: "Missing JSDoc @{{returns}} for function.",
data: {
returns: prefer.returns || "returns"
}
});
}
}

@Standard8
Copy link
Contributor Author

@rachx now I re-read the docs again(!), I think having returns optional in the requireReturn:false case makes sense.

Updating that section seems to make sense as well. Do you want to do a PR for that, or I could do one tomorrow?

@rachx
Copy link
Contributor

rachx commented Jun 15, 2018

@Standard8 You can make the PR 👍

Standard8 added a commit to Standard8/eslint that referenced this issue Jun 15, 2018
Standard8 added a commit to Standard8/eslint that referenced this issue Jun 15, 2018
…t#10386)

Change valid-jsdoc to allow returns to be optional for async functions in the requireReturn:false case.
matt-riley pushed a commit to matt-riley/gql_boilerplate that referenced this issue Jun 26, 2018
## Version **5.0.1** of **[eslint](https://github.com/eslint/eslint)** was just published.

<table>
  <tr>
    <th align=left>
      Dependency
    </th>
    <td>
      <code>[eslint](https://github.com/eslint/eslint)</code>
    </td>
  </tr>
  <tr>
      <th align=left>
       Current Version
      </th>
      <td>
        5.0.0
      </td>
    </tr>
  <tr>
    <th align=left>
      Type
    </th>
    <td>
      devDependency
    </td>
  </tr>
</table>



The version **5.0.1** is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

It might be worth looking into these changes and trying to get this project onto the latest version of eslint.

If you have a solid test suite and good coverage, a passing build is a strong indicator that you can take advantage of these changes directly by merging the proposed change into your project. If the build fails or you don’t have such unconditional trust in your tests, this branch is a great starting point for you to work on the update.


---


<details>
<summary>Release Notes</summary>
<strong>v5.0.1</strong>

<ul>
<li><a class="commit-link" href="https://urls.greenkeeper.io/eslint/eslint/commit/196c1021c070e52259f2ceccc4f141bdb7456c60"><tt>196c102</tt></a> Fix: valid-jsdoc should allow optional returns for async (fixes <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="324854620" data-permission-text="Issue title is private" data-url="eslint/eslint#10386" href="https://urls.greenkeeper.io/eslint/eslint/issues/10386">#10386</a>) (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="332691684" data-permission-text="Issue title is private" data-url="eslint/eslint#10480" href="https://urls.greenkeeper.io/eslint/eslint/pull/10480">#10480</a>) (Mark Banner)</li>
<li><a class="commit-link" href="https://urls.greenkeeper.io/eslint/eslint/commit/4c823bdf42af8f9a9fcf47473e48a99758881cf4"><tt>4c823bd</tt></a> Docs: Fix max-lines-per-function correct code's max value (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="335122700" data-permission-text="Issue title is private" data-url="eslint/eslint#10513" href="https://urls.greenkeeper.io/eslint/eslint/pull/10513">#10513</a>) (Rhys Bower)</li>
</ul>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 4 commits.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/caeb223c4f7b0b6fe35e5348ae0df4c6446b5bed"><code>caeb223</code></a> <code>5.0.1</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/125dc34637138b240976f0f76ef53476b15f08b3"><code>125dc34</code></a> <code>Build: changelog update for 5.0.1</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/196c1021c070e52259f2ceccc4f141bdb7456c60"><code>196c102</code></a> <code>Fix: valid-jsdoc should allow optional returns for async (fixes #10386) (#10480)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/4c823bdf42af8f9a9fcf47473e48a99758881cf4"><code>4c823bd</code></a> <code>Docs: Fix max-lines-per-function correct code's max value (#10513)</code></li>
</ul>
<p>See the <a href="https://urls.greenkeeper.io/eslint/eslint/compare/36ced0afca1bc0e1cbbc13cbeaacf9e7cf00841f...caeb223c4f7b0b6fe35e5348ae0df4c6446b5bed">full diff</a></p>
</details>

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 23, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

3 participants