chore: restore eslint-plugin-jsdoc rules #316
Conversation
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
This is lovely, thank you for the cleanup!
I have a couple of touchup requests, but generally am very 👍 on the direction of changes. These'll be very nice for understanding the code better.
| * create the array to return. | ||
| */ | ||
| const asyncIterable = await flatTraverse(items); | ||
| const asyncIterable = flatTraverse(items); |
There was a problem hiding this comment.
Technically, this is a behavior change. It makes me a little nervous to include any in a chore:, even if it's a no-op. IMO this is fine as flatTraverse isn't async and doesn't return a Promise. Just calling out as something to keep an eye on.
| * @param {any} value1 The value from the first object key. | ||
| * @param {any} value2 The value from the second object key. | ||
| * @returns {any} The second value. |
There was a problem hiding this comment.
[Bug] {any} isn't correct here. This is a generic function so there should be a type parameter:
| * @param {any} value1 The value from the first object key. | |
| * @param {any} value2 The value from the second object key. | |
| * @returns {any} The second value. | |
| * @template Value | |
| * @param {Value} value1 The value from the first object key. | |
| * @param {Value} value2 The value from the second object key. | |
| * @returns {Value} The second value. |
Quick reference as I always have to look these up ~ https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#template
(here and elsewhere in the PR in functions that should be generic)
| /** | ||
| * Validates that a value is an array. | ||
| * @param {*} value The value to validate. | ||
| * @param {any} value The value to validate. |
There was a problem hiding this comment.
[Bug] This won't change anything specifically here, but preferring unknown is generally a better practice and leads to safer default behaviors:
| * @param {any} value The value to validate. | |
| * @param {unknown} value The value to validate. |
(here and elsewhere in the file)
| * Shorthand for checking if a value is a string. | ||
| * @param {any} value The value to check. | ||
| * @returns {boolean} True if a string, false if not. | ||
| * @returns {value is string} True if a string, false if not. |
There was a problem hiding this comment.
[Praise] Nice "extra credit" to get the type predicate!
| /** | ||
| * Asserts that a given value is an array. | ||
| * @param {*} value The value to check. | ||
| * @param {any} value The value to check. |
There was a problem hiding this comment.
[Bug] This won't change anything specifically here, but preferring unknown is generally a better practice and leads to safer default behaviors:
| * @param {any} value The value to check. | |
| * @param {unknown} value The value to check. |
(here and elsewhere that uses any for an unknown-typed value)
There was a problem hiding this comment.
[Praise] Nice cleanup here on removing the ...s, etc. too 🙂
Prerequisites checklist
What is the purpose of this pull request?
This PR restores the eslint-plugin-jsdoc rules that were previously disabled, so that JavaScript files once again receive the intended documentation linting behavior from eslint-config-eslint.
What changes did you make? (Give an overview)
Removed the blanket disable of jsdoc rules and cleaned up the configuration structure in
eslint.config.js.Related Issues
Is there anything you'd like reviewers to focus on?