Fix eslint issues #25352

renatho · 2020-09-15T22:04:05Z

Description

This PR fixes some Eslint issues related to PR #25300.
It still contains some issues related to tests (tests without assertion and jasmine global - see screenshots) which I preferred don't touch for now because we could follow different approaches and I'm not sure what would fit better.

I separated the changes in different commits, where the first one is about indentation only. In the second one, I also added some arguments that were missing with their types. I identified the type through the code interpretation, so I'd appreciate a more detailed check here: 598aea5

How has this been tested?

Running npm run lint-js in Gutenberg code.

Screenshots

The remaining issues (warnings that were present before the changes too):

Types of changes

Eslint fixes related to JSDoc.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

noahtallen

This looks fine just looking at /components/env

noahtallen · 2020-09-15T22:47:24Z

packages/env/lib/config/config.js

+ * @property {string}         workDirectoryPath       Path to the work directory located in ~/.wp-env.
+ * @property {string}         dockerComposeConfigPath Path to the docker-compose.yml file.
+ * @property {boolean}        detectedLocalConfig     If true, wp-env detected local config and used it.
+ * @property {Object.<string, WPServiceConfig>}       env                     Specific config for different environments.


this seems like a weird fix. Why it is aligning the variables with the type parameter? 🤔

Good catch @noahtallen! It's an issue with the plugin that I haven't seen before. For now, I just updated removing the space, but I'll submit a PR to the eslint-jsdoc-plugin with a fix in the next few days.

ntwb

Thanks, left a few comments, as we are touching these lines now, helping our future selves for Git blame, maybe adding the trailing . period to the @param descriptions now that are missing them would be good (It appears there are not many instances where these are missing)

I got through around ~25% of the files, can we make those changes noted in the review and then I can come back for a full review pass of all the files

Edit: It may in fact we worthwhile trying to use Prettier prettier-plugin-jsdoc here to update the docs s mentioned here in #24984 (comment) as this may be signifantly quicker for both this PR and future code written will be formatted automatically rather than a devloper pushing up code that then fails linting

ntwb · 2020-09-16T05:36:46Z

bin/packages/build.js

+ * @param {string} file File name
 * @return {string}      Package name


If @return descriptions are not destined to be aligned with @param descriptions then I think this also requires some changes:

This aligns the descriptions of both lines:

Suggested change

* @param {string} file File name

* @return {string} Package name

* @param {string} file File name

* @return {string} Package name

Or, this removes the additional whitespece preceding the @return description

* @param {string} file File name * @return {string} Package name

Possibly, another convention observed further below is a blank line between the last @param and the @Raturn:

* @param {string} file File name * * @return {string} Package name

ntwb · 2020-09-16T05:39:20Z

bin/plugin/config.js

+ * @property {string} versionMilestoneFormat  printf template for milestone
 *                                           version name. Expected to be called
 *                                           with a merged object of the config
 *                                           and semver-parsed version parts.


Suggested change

* @property {string} versionMilestoneFormat printf template for milestone

* version name. Expected to be called

* with a merged object of the config

* and semver-parsed version parts.

* @property {string} versionMilestoneFormat printf template for milestone

* version name. Expected to be called

* with a merged object of the config

* and semver-parsed version parts.

Identation of the subsequent lines should align with the start of the first comment

ntwb · 2020-09-16T05:41:12Z

bin/plugin/lib/git.js

@@ -47,7 +47,7 @@ async function commit( gitWorkingDirectoryPath, message, filesToAdd = [] ) {
 * Creates a local branch.
 *
 * @param {string} gitWorkingDirectoryPath Local repository path.
- * @param {string} branchName Branch Name
+ * @param {string} branchName              Branch Name


Not a blocker, but we should look at standardizing either including or excluding the . period at the end of the @param description, PHP Coding Standards require this, so I'd lean toward that also for JS (A goal is to align the CS between languages as best we can when suited)

ntwb · 2020-09-16T05:42:41Z

packages/annotations/src/format/annotation.js

- * @param {Object} record The record to apply annotations to.
- * @param {Array} annotations The annotation to apply.
+ * @param {Object} record      The record to apply annotations to.
+ * @param {Array}  annotations The annotation to apply.
 * @return {Object} A record with the annotations applied.


FYI Only: An example where there's no blank line between the last @param and the @return and the @return description is not aligned with the @param descriptions

ntwb · 2020-09-16T05:43:52Z

packages/autop/src/index.js

+ * @param {boolean} br   Optional. If set, will convert all remaining line-
 *                          breaks after paragraphing. Default true.


Suggested change

* @param {boolean} br Optional. If set, will convert all remaining line-

* breaks after paragraphing. Default true.

* @param {boolean} br Optional. If set, will convert all remaining line-

* breaks after paragraphing. Default true.

The original second line aligned with the start of the original description, seems like this should be updated here also

ntwb · 2020-09-16T05:49:59Z

packages/block-editor/src/components/inner-blocks/use-inner-block-template-sync.js

+ * @param {string}  clientId                       The block client ID.
+ * @param {Object}  template                       The template to match.
+ * @param {string}  templateLock                   The template lock state for the inner blocks. For


This needs updating and the lines below to line 31, looks like autofix missed the @param on line 28

ntwb · 2020-09-16T05:51:04Z

packages/block-editor/src/components/inserter/hooks/use-block-types-state.js

- * @param {string=}  rootClientId        Insertion's root client ID.
- * @param {Function} onInsert            function called when inserter a list of blocks.
+ * @param {string=}  rootClientId Insertion's root client ID.
+ * @param {Function} onInsert     function called when inserter a list of blocks.


Suggested change

* @param {Function} onInsert function called when inserter a list of blocks.

* @param {Function} onInsert Function called when inserter a list of blocks.

Should descriptions always begin with a Capital letter? It appears to be the convention and is also a PHP coding standard

ntwb · 2020-09-16T05:51:45Z

packages/block-editor/src/components/provider/use-block-sync.js

@@ -40,25 +40,25 @@ import { useRegistry } from '@wordpress/data';
 *   controllers.


More examples in this file of second line descriptions not aligned with the first line

ntwb · 2020-09-16T05:52:35Z

packages/block-editor/src/hooks/align.js

@@ -80,7 +80,7 @@ export function getValidAlignments(
 /**
 * Filters registered block settings, extending attributes to include `align`.
 *
- * @param  {Object} settings Original block settings
+ * @param {Object} settings Original block settings
 * @return {Object}          Filtered block settings


Suggested change

* @return {Object} Filtered block settings

* @return {Object} Filtered block settings

Remove additional whitespace for @return descriptions

ntwb · 2020-09-16T05:53:47Z

packages/block-editor/src/store/reducer.js

+ * @param {Object}  blocksOrder           Object that maps block client IDs to a list of
 *                               nested block client IDs.


Suggested change

* @param {Object} blocksOrder Object that maps block client IDs to a list of

* nested block client IDs.

* @param {Object} blocksOrder Object that maps block client IDs to a list of

* nested block client IDs.

renatho · 2020-09-16T19:33:52Z

Thank you so much for the review @ntwb!!

As I mentioned here: #25300 (comment), I think I'll keep this PR in stand by for now until going with that fixes.

But it worthed a lot to get some more cases and maybe include some of them in the rule logic.

I'm closing this for now and I'll open a new one when it's ready. It'll be easier because it'll have a lot of conflicts.

renatho mentioned this pull request Sep 15, 2020

Update Eslint JSDoc package, introducing JSDoc line alignment check #25300

Merged

6 tasks

renatho force-pushed the fix/eslint-rules branch from 598aea5 to 3319df2 Compare September 15, 2020 22:16

renatho marked this pull request as ready for review September 15, 2020 22:17

renatho requested review from adamsilverstein, ajitbohra, ajlende, atimmer, cameronvoell, chrisvanpatten, ellatrix, epiqueras, etoledom, guarani, gziolo, nerrad, noahtallen, oandregal, ntwb, Soean, swissspidy, talldan and youknowriad as code owners September 15, 2020 22:17

renatho force-pushed the fix/eslint-rules branch from 3319df2 to f12cc4a Compare September 15, 2020 22:30

noahtallen approved these changes Sep 15, 2020

View reviewed changes

renatho added 3 commits September 15, 2020 20:12

Autofix eslint rules

ad53a09

Fix eslint JSDoc missing params

e937887

Update JSDoc removing spaces from type

05f4f13

renatho force-pushed the fix/eslint-rules branch from f12cc4a to 05f4f13 Compare September 15, 2020 23:12

ntwb requested changes Sep 16, 2020

View reviewed changes

renatho closed this Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix eslint issues #25352

Fix eslint issues #25352

renatho commented Sep 15, 2020 •

edited

Loading

noahtallen left a comment

noahtallen Sep 15, 2020

renatho Sep 15, 2020

ntwb left a comment •

edited

Loading

ntwb Sep 16, 2020

ntwb Sep 16, 2020

ntwb Sep 16, 2020

ntwb Sep 16, 2020

ntwb Sep 16, 2020

ntwb Sep 16, 2020

ntwb Sep 16, 2020

ntwb Sep 16, 2020

ntwb Sep 16, 2020

ntwb Sep 16, 2020

renatho commented Sep 16, 2020

		* @param {string} file File name
		* @return {string} Package name

		* @param {boolean} br Optional. If set, will convert all remaining line-
		* breaks after paragraphing. Default true.

	* @param {Function} onInsert function called when inserter a list of blocks.
	* @param {Function} onInsert Function called when inserter a list of blocks.

		@@ -40,25 +40,25 @@ import { useRegistry } from '@wordpress/data';
		* controllers.

	* @return {Object} Filtered block settings
	* @return {Object} Filtered block settings

		* @param {Object} blocksOrder Object that maps block client IDs to a list of
		* nested block client IDs.

Fix eslint issues #25352

Fix eslint issues #25352

Conversation

renatho commented Sep 15, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

noahtallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntwb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho commented Sep 16, 2020

renatho commented Sep 15, 2020 •

edited

Loading

ntwb left a comment •

edited

Loading