Make WordPress Core

Opened 8 years ago

Closed 6 years ago

Last modified 5 years ago

#35465 closed enhancement (fixed)

Improve wp.Backbone docs

Reported by: ericlewis's profile ericlewis Owned by: gma992's profile gma992
Milestone: 5.1 Priority: normal
Severity: normal Version: 3.5
Component: General Keywords: good-first-bug
Focuses: javascript, docs Cc:

Description (last modified by ericlewis)

Let's convert the docblocks to JSDoc.

Attachments (8)

35465.diff (2.3 KB) - added by ericlewis 8 years ago.
35465.1.diff (11.3 KB) - added by gma992 8 years ago.
Converted some more docblocks and added selector and views params
35465.2.diff (11.5 KB) - added by adamsilverstein 7 years ago.
35465.3.diff (12.8 KB) - added by adamsilverstein 7 years ago.
35465.4.diff (15.5 KB) - added by atimmer 6 years ago.
Patch with all docblocks present
35465.5.diff (16.7 KB) - added by atimmer 6 years ago.
Patch with all @since tags.
35465.6.diff (2.8 KB) - added by birgire 6 years ago.
35465.7.diff (4.4 KB) - added by birgire 6 years ago.

Download all attachments as: .zip

Change History (32)

#1 @ericlewis
8 years ago

  • Description modified (diff)

@ericlewis
8 years ago

#2 @ocean90
8 years ago

  • Focuses docs added
  • Keywords needs-refresh good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

+1. 35465.diff is a good start.

@gma992
8 years ago

Converted some more docblocks and added selector and views params

#3 @gma992
8 years ago

  • Keywords needs-refresh removed

#4 @DrewAPicture
8 years ago

  • Keywords has-patch added

#5 @DrewAPicture
7 years ago

  • Owner set to gma992
  • Status changed from new to assigned

Assigning ownership to mark the good-first-bug as "claimed".

#6 @adamsilverstein
7 years ago

35465.2.diff refresh against trunk

#7 @adamsilverstein
7 years ago

  • Keywords needs-patch added; has-patch removed

35465.3.diff:

  • whitspace fixes (spaces for multiline indent)
  • add explanation of options parameter
  • still needs more work, some sections are missing docs

This ticket was mentioned in Slack in #core-js by mte90. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


6 years ago

#10 @netweb
6 years ago

  • Milestone changed from Future Release to 5.0

This ticket was mentioned in Slack in #core-js by atimmer. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-js by atimmer. View the logs.


6 years ago

@atimmer
6 years ago

Patch with all docblocks present

@atimmer
6 years ago

Patch with all @since tags.

#13 @atimmer
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42993:

Docs: Improve JSDoc for wp-includes/js/wp-backbone.js.

Props ericlewis, gma992, adamsilverstein.
Fixes #35465.

@birgire
6 years ago

#14 @birgire
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

35465.6.diff is a suggestion that:

  • Aligns related descriptions.
  • Adds full stops to some summaries.
  • Capitalizes object in * @param {array|object}
  • Changes a two line // comment into a multiline comment using /*...*/.
  • Changes $returns to $return.

Notes:

  • I noticed further {?Object} and {?number}here. Should we remove the question marks?


  • Are the > and – really needed here?

#15 follow-up: @adamsilverstein
6 years ago

I noticed further {?Object} and {?number}here. Should we remove the question marks?

The question mark indicates the field is optional. If the Object is optional it can stay. I don't see a number parameter in that function al all though, seems like that line can be removed?

Are the > and – really needed here?

These do seem out of place. I think you can leave out the '>' items or change them both to a regular '-' dash.

@birgire
6 years ago

#16 in reply to: ↑ 15 @birgire
6 years ago

Replying to adamsilverstein:

The question mark indicates the field is optional. If the Object is optional it can stay.

Thanks @adamsilverstein, I skimmed through:

https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/javascript/

when I noticed ?Object and ?number, but I didn't find a reference to it there.

I think it could be helpful to mention it in the Handbook.

I don't see a number parameter in that function al all though, seems like that line can be removed?

I wasn't exactly sure regarding that, so I didn't change that.

35465.7.diff

  • Removes > from description.
  • Replaces – with - in description.
  • Moves a word up to the line above, in description, for better readability.
  • Makes a single alignment.

#17 follow-up: @netweb
6 years ago

I've created #43828 that adds a JSDoc ESLint script, it can be run by downloading and applying the patch and updating the dependencies by running npm install and npm run lint:jsdoc to run the script.

The configuration comes by way of previous existing Gutenberg work, references are linked in #43828

I'll get the handbook updated to document all this.

cc @adamsilverstein @atimmer @birgire

#18 in reply to: ↑ 17 ; follow-up: @birgire
6 years ago

Replying to netweb:

I've created #43828 that adds a JSDoc ESLint script, it can be run by downloading and applying the patch and updating the dependencies by running npm install and npm run lint:jsdoc to run the script.

Great, I tested running npm install and npm run lint:jsdoc, with output for all files:

> WordPress@5.0.0 lint:jsdoc /www/sites/test.local/wp
> eslint . --config .eslintrc-jsdoc.js

...
/path/to/src/wp-includes/js/wp-backbone.js
   23:14  error  Use 'Array' instead of 'array'      valid-jsdoc
   38:15  error  Use 'Array' instead of 'array'      valid-jsdoc
   44:3   error  Missing JSDoc return description    valid-jsdoc
   54:15  error  Use 'Array' instead of 'array'      valid-jsdoc
   80:3   error  Missing JSDoc return type           valid-jsdoc
  111:14  error  Use 'Array' instead of 'array'      valid-jsdoc
  177:3   error  Missing JSDoc return type           valid-jsdoc
  200:14  error  Use 'Array' instead of 'array'      valid-jsdoc
  200:20  error  Use 'Object' instead of 'object'    valid-jsdoc
  231:14  error  Use 'Array' instead of 'array'      valid-jsdoc
  231:20  error  Use 'Object' instead of 'object'    valid-jsdoc
  408:14  error  Use 'Array' instead of 'array'      valid-jsdoc
  408:20  error  Use 'Object' instead of 'object'    valid-jsdoc
  455:6   error  Use @return instead                 valid-jsdoc
  499:3   error  Missing JSDoc @return for function  valid-jsdoc
  519:6   error  Use @return instead                 valid-jsdoc
  544:6   error  Use @return instead                 valid-jsdoc
...

â 3213 problems (3213 errors, 0 warnings)
  1935 errors, 0 warnings potentially fixable with the `--fix` option.

ps: how would we run this on a specific .js file?

Last edited 6 years ago by birgire (previous) (diff)

#19 in reply to: ↑ 18 @netweb
6 years ago

Replying to birgire:

ps: how would we run this on a specific .js file?

For the time being, this should work:

  • ./node_modules/.bin/eslint --config .eslintrc-jsdoc.js src/wp-includes/js/wp-backbone.js

And, if you tack --fix onto the end it will automagically fix 13 of the 17 errors for you ✨

  • ./node_modules/.bin/eslint --config .eslintrc-jsdoc.js src/wp-includes/js/wp-backbone.js --fix

#20 @birgire
6 years ago

Thank you @netweb 🎉

$ ./node_modules/.bin/eslint --config .eslintrc-jsdoc.js src/wp-includes/js/wp-backbone.js

/path/to/src/wp-includes/js/wp-backbone.js
   23:14  error  Use 'Array' instead of 'array'      valid-jsdoc
   38:15  error  Use 'Array' instead of 'array'      valid-jsdoc
   44:3   error  Missing JSDoc return description    valid-jsdoc
   54:15  error  Use 'Array' instead of 'array'      valid-jsdoc
   80:3   error  Missing JSDoc return type           valid-jsdoc
  111:14  error  Use 'Array' instead of 'array'      valid-jsdoc
  177:3   error  Missing JSDoc return type           valid-jsdoc
  200:14  error  Use 'Array' instead of 'array'      valid-jsdoc
  200:20  error  Use 'Object' instead of 'object'    valid-jsdoc
  231:14  error  Use 'Array' instead of 'array'      valid-jsdoc
  231:20  error  Use 'Object' instead of 'object'    valid-jsdoc
  408:14  error  Use 'Array' instead of 'array'      valid-jsdoc
  408:20  error  Use 'Object' instead of 'object'    valid-jsdoc
  455:6   error  Use @return instead                 valid-jsdoc
  499:3   error  Missing JSDoc @return for function  valid-jsdoc
  519:6   error  Use @return instead                 valid-jsdoc
  544:6   error  Use @return instead                 valid-jsdoc

â 17 problems (17 errors, 0 warnings)
  13 errors, 0 warnings potentially fixable with the `--fix` option.

 $ ./node_modules/.bin/eslint --config .eslintrc-jsdoc.js src/wp-includes/js/wp-backbone.js --fix

/path/to/src/wp-includes/js/wp-backbone.js
   44:3  error  Missing JSDoc return description    valid-jsdoc
   80:3  error  Missing JSDoc return type           valid-jsdoc
  177:3  error  Missing JSDoc return type           valid-jsdoc
  499:3  error  Missing JSDoc @return for function  valid-jsdoc

â 4 problems (4 errors, 0 warnings)

#21 @atimmer
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 42996:

Docs: Improve JSDoc for wp-includes/js/wp-backbone.js.

Changes some additional docs after [42993].

Props birgire, adamsilverstein, netweb.
Fixes #35465.

#22 @atimmer
6 years ago

@birgire Thank you for your contribution!

The ?number argument refers to a property that can be set on the object. When you document it like I did there JSDoc can recognize that the argument is a property on the object.

#23 @johnbillion
5 years ago

  • Milestone changed from 5.0 to 5.1

#24 @johnbillion
5 years ago

  • Keywords needs-patch removed
Note: See TracTickets for help on using tickets.