Issue 29859565: Issue 6741 - Use ES2015 classes in lib/downloader.js (Closed)
|
Created:
Aug. 20, 2018, 11:07 p.m. by Jon Sonesen Modified:
Aug. 27, 2018, 9:29 p.m. Reviewers:
Manish Jethani CC:
kzar Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 6741 - Use ES2015 classes in lib/downloader.js
Patch Set 1 #Patch Set 2 : Remove excess whitespace #
Total comments: 10
Patch Set 3 : Address PS2 comments #Patch Set 4 : Insert blank line before jsdocs for _timer #
Total comments: 6
Patch Set 5 : Address PS4 comments #
Total comments: 11
Patch Set 6 : Address PS5 comments #Patch Set 7 : Address PS6 comment #
Total comments: 2
Patch Set 8 : Address PS7 comments #MessagesTotal messages: 13
Not sure if anything in the classes needed to be defined as static, or private outside of the constructor. Tried both ways and this is the way the tests pass.
On 2018/08/20 23:13:18, Jon Sonesen wrote: > Not sure if anything in the classes needed to be defined as static, or private > outside of the constructor. Tried both ways and this is the way the tests pass. There's no static or private ES2015 unfortunately (may be coming soon though), which is why we dragged our feet a bit on adopting the new class syntax. On balance I thought it was probably a good idea to do it so we're doing it in core first.
https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:32: * Creates a new downloader instance. This JSDoc comment is in fact for the constructor; we should move it to just above `constructor()` https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:46: * @type {nsITimer} So you see there was always an inconsistency between the JSDoc and the line below it: in this case, it says this property cannot be null (no `?` next to the type), but we're setting it to null anyway. That's because the real initialization happens in the constructor. But now we can do this properly so that the initialization and the JSDoc comment are in the same place. I think we should set this property once below and move the JSDoc comment there. I've been thinking of a convention: (1) first, properties with default values like null, true, etc.; (2) next properties with values from the constructor arguments; (3) last, properties with values assigned based on some internal logic. It's rough, but we can use this for now. https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:360: } Blank line after `}` https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:364: * An object that can be downloaded by the downloadable Same as above, this is JSDoc for the constructor.
https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:32: * Creates a new downloader instance. On 2018/08/21 03:56:02, Manish Jethani wrote: > This JSDoc comment is in fact for the constructor; we should move it to just > above `constructor()` Acknowledged. I wasn't sure about that so thanks for clarifying! https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:46: * @type {nsITimer} On 2018/08/21 03:56:02, Manish Jethani wrote: > So you see there was always an inconsistency between the JSDoc and the line > below it: in this case, it says this property cannot be null (no `?` next to the > type), but we're setting it to null anyway. That's because the real > initialization happens in the constructor. > > But now we can do this properly so that the initialization and the JSDoc comment > are in the same place. I think we should set this property once below and move > the JSDoc comment there. > > I've been thinking of a convention: (1) first, properties with default values > like null, true, etc.; (2) next properties with values from the constructor > arguments; (3) last, properties with values assigned based on some internal > logic. It's rough, but we can use this for now. Yeah, I also wasn't sure about that either and I like the convention you outlined, I considered that same convention but figured it could be clarified here. https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:360: } On 2018/08/21 03:56:02, Manish Jethani wrote: > Blank line after `}` Do you mean insert a blank line here? https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:364: * An object that can be downloaded by the downloadable On 2018/08/21 03:56:02, Manish Jethani wrote: > Same as above, this is JSDoc for the constructor. Acknowledged.
https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:360: } On 2018/08/21 19:10:01, Jon Sonesen wrote: > On 2018/08/21 03:56:02, Manish Jethani wrote: > > Blank line after `}` > > Do you mean insert a blank line here? Yes, I meant we should have a blank line after `class { ... }` https://codereview.adblockplus.org/29859565/diff/29860587/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29860587/lib/downloader.js#n... lib/downloader.js:35: * @param {Function} dataSource Another JSDoc convention that I think that we should follow is to use the lower-case type name for primitive types [1]. In this case, let's make it `function` instead of `Function` (throughout the file). [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Data_... https://codereview.adblockplus.org/29859565/diff/29860587/lib/downloader.js#n... lib/downloader.js:362: * An object that can be downloaded by the downloadable I think this should be "Creates an object that can be downloaded by the downloader." (Note: downloader, not downloadable (that's a mistake), and a period at the end.) https://codereview.adblockplus.org/29859565/diff/29860587/lib/downloader.js#n... lib/downloader.js:371: this.url = null; Let's also just assign `this.url` directly at the end of the constructor and move the JSDoc comment there.
https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29859568/lib/downloader.js#n... lib/downloader.js:360: } On 2018/08/22 06:13:34, Manish Jethani wrote: > On 2018/08/21 19:10:01, Jon Sonesen wrote: > > On 2018/08/21 03:56:02, Manish Jethani wrote: > > > Blank line after `}` > > > > Do you mean insert a blank line here? > > Yes, I meant we should have a blank line after `class { ... }` Done. https://codereview.adblockplus.org/29859565/diff/29860587/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29860587/lib/downloader.js#n... lib/downloader.js:35: * @param {Function} dataSource On 2018/08/22 06:13:34, Manish Jethani wrote: > Another JSDoc convention that I think that we should follow is to use the > lower-case type name for primitive types [1]. In this case, let's make it > `function` instead of `Function` (throughout the file). > > [1]: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Data_... Done. https://codereview.adblockplus.org/29859565/diff/29860587/lib/downloader.js#n... lib/downloader.js:362: * An object that can be downloaded by the downloadable On 2018/08/22 06:13:35, Manish Jethani wrote: > I think this should be "Creates an object that can be downloaded by the > downloader." (Note: downloader, not downloadable (that's a mistake), and a > period at the end.) Done. https://codereview.adblockplus.org/29859565/diff/29860587/lib/downloader.js#n... lib/downloader.js:371: this.url = null; On 2018/08/22 06:13:35, Manish Jethani wrote: > Let's also just assign `this.url` directly at the end of the constructor and > move the JSDoc comment there. Done. https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:418: exports.Downloadable = Downloadable; I thought having a blank line, the addition to exports and another blank line looked a bit weird, and I kinda like having definitions complete, then followed by assignments as far as readability goes.
https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:36: * function that will yield downloadable objects on each check This should still be capital F? It's not the type name here, just prose (similar to "Number of milliseconds ..." below). https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:72: * @type {function} Let's change the type here to `function?` (also below wherever it's set to null by default). https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:96: * function that will yield downloadable objects on each check. This too. https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:368: * @type {string} Since redirectURL can be null (starts out as null by default, in fact), let's change the type here to `string?`. https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:418: exports.Downloadable = Downloadable; On 2018/08/22 18:18:51, Jon Sonesen wrote: > I thought having a blank line, the addition to exports and another blank line > looked a bit weird, and I kinda like having definitions complete, then followed > by assignments as far as readability goes. Sure, this is how I do it too for my personal projects, but here we should try to be consistent with the rest of the codebase.
Thanks for the quick response Manish https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:36: * function that will yield downloadable objects on each check On 2018/08/23 15:03:39, Manish Jethani wrote: > This should still be capital F? It's not the type name here, just prose (similar > to "Number of milliseconds ..." below). Done. I used sed replacement carelessly to do the change, my bad. https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:72: * @type {function} On 2018/08/23 15:03:39, Manish Jethani wrote: > Let's change the type here to `function?` (also below wherever it's set to null > by default). Done. https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:96: * function that will yield downloadable objects on each check. On 2018/08/23 15:03:39, Manish Jethani wrote: > This too. Done. https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:368: * @type {string} On 2018/08/23 15:03:39, Manish Jethani wrote: > Since redirectURL can be null (starts out as null by default, in fact), let's > change the type here to `string?`. Done. https://codereview.adblockplus.org/29859565/diff/29861580/lib/downloader.js#n... lib/downloader.js:418: exports.Downloadable = Downloadable; On 2018/08/23 15:03:39, Manish Jethani wrote: > On 2018/08/22 18:18:51, Jon Sonesen wrote: > > I thought having a blank line, the addition to exports and another blank line > > looked a bit weird, and I kinda like having definitions complete, then > followed > > by assignments as far as readability goes. > > Sure, this is how I do it too for my personal projects, but here we should try > to be consistent with the rest of the codebase. Done.
Let's also replace all `@return` with `@returns` in the file. Otherwise LGTM
One more comment ... https://codereview.adblockplus.org/29859565/diff/29863599/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29863599/lib/downloader.js#n... lib/downloader.js:346: * @returns {Array} soft and hard expiration interval Sorry, one more thing: Can we change this to `Array.<number>`?
No problem, thanks! https://codereview.adblockplus.org/29859565/diff/29863599/lib/downloader.js File lib/downloader.js (right): https://codereview.adblockplus.org/29859565/diff/29863599/lib/downloader.js#n... lib/downloader.js:346: * @returns {Array} soft and hard expiration interval On 2018/08/25 07:46:11, Manish Jethani wrote: > Sorry, one more thing: Can we change this to `Array.<number>`? Done.
LGTM 
|
