Issue 29855595: Issue 6741 - Use ES2015 classes in lib/events.js (Closed)
|
Created:
Aug. 14, 2018, 11:30 p.m. by Jon Sonesen Modified:
Aug. 22, 2018, 6:06 p.m. Reviewers:
Manish Jethani CC:
kzar Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionThis change updates lib/events.js to use ES2015 classes and also updates doc strings to be more consistent (issue 6676)
Patch Set 1 #Patch Set 2 : Capitalize JsDoc String #
Total comments: 13
Patch Set 3 : Address PS2 Comments #
Total comments: 2
Patch Set 4 : Address PS3 Comment #MessagesTotal messages: 11
Can you modify the summary to change "es6" to "ES2015" just for consistency? Also run `jsdoc lib/events.js` and open `out/index.html` and navigate to the EventEmitter class to see that it's all looking good. More comments inline ... https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js File lib/events.js (right): https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:23: * @class The @class keyword is redundant now, as far as I know. We can remove it. https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:25: exports.EventEmitter = class EventEmitter Let's declare it like this: class EventEmitter { ... } exports.EventEmitter = EventEmitter; Only because we have following this convention for function as well. Note that there's no semicolon in this case after class { ... } https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:45: } Let's leave a blank line after each method. https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:66: * @return {Promise} @return -> @returns (see comment below) https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:85: * @return {function[]} I'm trying to make the JSDoc comments consistent across core so let's also take this opportunity to do this (Trac issue #6676). @return -> @returns function[] -> Array.<function> https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:96: * @param {...*} [arg] Let's rename the JSDoc name of the parameter to "args" (since that's what it's called in the code).
On 2018/08/15 08:12:57, Manish Jethani wrote: > Can you modify the summary to change "es6" to "ES2015" just for consistency? Actually let's change it to "Use ES2015 classes in lib/events.js"
On 2018/08/15 08:13:53, Manish Jethani wrote: > On 2018/08/15 08:12:57, Manish Jethani wrote: > > Can you modify the summary to change "es6" to "ES2015" just for consistency? > > Actually let's change it to "Use ES2015 classes in lib/events.js" When running jsdoc it produces what I believe is the desired result, it classifies EventEmiiter as a class.
https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js File lib/events.js (right): https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:23: * @class On 2018/08/15 08:12:56, Manish Jethani wrote: > The @class keyword is redundant now, as far as I know. We can remove it. Acknowledged. https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:25: exports.EventEmitter = class EventEmitter On 2018/08/15 08:12:56, Manish Jethani wrote: > Let's declare it like this: > > class EventEmitter > { > ... > } > > exports.EventEmitter = EventEmitter; > > Only because we have following this convention for function as well. > > Note that there's no semicolon in this case after class { ... } Acknowledged. https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:45: } On 2018/08/15 08:12:56, Manish Jethani wrote: > Let's leave a blank line after each method. Acknowledged. https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:66: * @return {Promise} On 2018/08/15 08:12:56, Manish Jethani wrote: > @return -> @returns (see comment below) Acknowledged. https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:85: * @return {function[]} On 2018/08/15 08:12:56, Manish Jethani wrote: > I'm trying to make the JSDoc comments consistent across core so let's also take > this opportunity to do this (Trac issue #6676). > > @return -> @returns > > function[] -> Array.<function> Acknowledged. https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:96: * @param {...*} [arg] On 2018/08/15 08:12:57, Manish Jethani wrote: > Let's rename the JSDoc name of the parameter to "args" (since that's what it's > called in the code). Acknowledged.
https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js File lib/events.js (right): https://codereview.adblockplus.org/29855595/diff/29855598/lib/events.js#newco... lib/events.js:96: * @param {...*} [arg] On 2018/08/16 20:19:30, Jon Sonesen wrote: > On 2018/08/15 08:12:57, Manish Jethani wrote: > > Let's rename the JSDoc name of the parameter to "args" (since that's what it's > > called in the code). > > Acknowledged. The "type" was fine (i.e. {...*}), it's the name of the parameter that I wanted to change to args. I meant like this: @param {...*} [args] https://codereview.adblockplus.org/29855595/diff/29857619/lib/events.js File lib/events.js (right): https://codereview.adblockplus.org/29855595/diff/29857619/lib/events.js#newco... lib/events.js:65: * @param {string} name Can we also insert a blank line here before @param?
Oh dang, thanks for explaining https://codereview.adblockplus.org/29855595/diff/29857619/lib/events.js File lib/events.js (right): https://codereview.adblockplus.org/29855595/diff/29857619/lib/events.js#newco... lib/events.js:65: * @param {string} name On 2018/08/17 03:07:21, Manish Jethani wrote: > Can we also insert a blank line here before @param? Acknowledged.
LGTM
On 2018/08/20 07:57:46, Manish Jethani wrote: > LGTM I think you have to put a hyphen after "Issue 6741" in the commit messag
On 2018/08/20 17:48:04, Manish Jethani wrote: > On 2018/08/20 07:57:46, Manish Jethani wrote: > > LGTM > > I think you have to put a hyphen after "Issue 6741" in the commit messag Good catch, it's in the commit message just forgot it when updating the ticket
This change has landed [1] so you can close this review now. [1]: https://hg.adblockplus.org/adblockpluscore/rev/5fbc1e0a1156 
|
