Issue 29869571: Issue 6741 - Use ES2015 classes in lib/matcher.js (Closed)
|
Created:
Aug. 30, 2018, 3:53 p.m. by Jon Sonesen Modified:
Sept. 7, 2018, 4:08 a.m. Reviewers:
Manish Jethani Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 6741 - Use ES2015 classes in lib/matcher.js
Patch Set 1 #
Total comments: 27
Patch Set 2 : Address PS1 Comments #Patch Set 3 : Actually address ps1 comments #
Total comments: 9
Patch Set 4 : Address PS3 comments #
Total comments: 4
Patch Set 5 : Address PS4 Comments #MessagesTotal messages: 13
https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:29: /** I think this rather sounds like the description of the class itself, let's move it up to above the class keyword. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:45: this.clear(); Let's leave a line before this.clear(). Actually we should probably just instantiate the object here right away rather than calling the clear method. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:53: this.filterByKeyword = new Map(); Since these are Map objects now, we can just call the clear method on them (also see comment above). https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:108: * @return {string} keyword or an empty string if no keyword could be found Let's s/@return/@returns/ here and throughout the file. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:177: * @param {string} docDomain docDomain, thirdParty, sitekey, and specificOnly are all optional parameters. We should surround them with brackets. http://usejsdoc.org/tags-type.html https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:209: * @param {string} docDomain See comment above about optional parameters. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:216: * should be true if generic matches should be ignored Let's make this <code>true</code>. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:218: * matching filter or null Let's make this <code>null</code>. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:237: } Let's leave a blank line after the class definition. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:245: * Combines a matcher for blocking and exception rules, automatically sorts Let's move this to above the class definition. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:246: * rules into two Matcher instances. Let's change `Matcher` here to `{@link Matcher}` so it links to the class in the generate JSDoc. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:247: * @augments Matcher Let's remove @arguments here, it's outdated. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:270: * Lookup table of previous matchesAny results Let's make this `{@link Matcher#matchesAny}` https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:423: } Let's leave a blank line here. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:427: * Shared CombinedMatcher instance that should usually be used. Let's make this `{@link CombinedMatcher}`
https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:29: /** On 2018/08/31 13:01:34, Manish Jethani wrote: > I think this rather sounds like the description of the class itself, let's move > it up to above the class keyword. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:45: this.clear(); On 2018/08/31 13:01:35, Manish Jethani wrote: > Let's leave a line before this.clear(). > > Actually we should probably just instantiate the object here right away rather > than calling the clear method. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:53: this.filterByKeyword = new Map(); On 2018/08/31 13:01:35, Manish Jethani wrote: > Since these are Map objects now, we can just call the clear method on them (also > see comment above). Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:108: * @return {string} keyword or an empty string if no keyword could be found On 2018/08/31 13:01:34, Manish Jethani wrote: > Let's s/@return/@returns/ here and throughout the file. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:177: * @param {string} docDomain On 2018/08/31 13:01:34, Manish Jethani wrote: > docDomain, thirdParty, sitekey, and specificOnly are all optional parameters. We > should surround them with brackets. > > http://usejsdoc.org/tags-type.html Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:209: * @param {string} docDomain On 2018/08/31 13:01:35, Manish Jethani wrote: > See comment above about optional parameters. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:216: * should be true if generic matches should be ignored On 2018/08/31 13:01:35, Manish Jethani wrote: > Let's make this <code>true</code>. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:218: * matching filter or null On 2018/08/31 13:01:34, Manish Jethani wrote: > Let's make this <code>null</code>. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:237: } On 2018/08/31 13:01:34, Manish Jethani wrote: > Let's leave a blank line after the class definition. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:270: * Lookup table of previous matchesAny results On 2018/08/31 13:01:35, Manish Jethani wrote: > Let's make this `{@link Matcher#matchesAny}` Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:423: } On 2018/08/31 13:01:34, Manish Jethani wrote: > Let's leave a blank line here. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:427: * Shared CombinedMatcher instance that should usually be used. On 2018/08/31 13:01:35, Manish Jethani wrote: > Let's make this `{@link CombinedMatcher}` Done.
https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:29: /** On 2018/08/31 13:01:34, Manish Jethani wrote: > I think this rather sounds like the description of the class itself, let's move > it up to above the class keyword. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:45: this.clear(); On 2018/08/31 13:01:35, Manish Jethani wrote: > Let's leave a line before this.clear(). > > Actually we should probably just instantiate the object here right away rather > than calling the clear method. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:53: this.filterByKeyword = new Map(); On 2018/08/31 13:01:35, Manish Jethani wrote: > Since these are Map objects now, we can just call the clear method on them (also > see comment above). Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:108: * @return {string} keyword or an empty string if no keyword could be found On 2018/08/31 13:01:34, Manish Jethani wrote: > Let's s/@return/@returns/ here and throughout the file. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:177: * @param {string} docDomain On 2018/08/31 13:01:34, Manish Jethani wrote: > docDomain, thirdParty, sitekey, and specificOnly are all optional parameters. We > should surround them with brackets. > > http://usejsdoc.org/tags-type.html Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:209: * @param {string} docDomain On 2018/08/31 13:01:35, Manish Jethani wrote: > See comment above about optional parameters. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:216: * should be true if generic matches should be ignored On 2018/08/31 13:01:35, Manish Jethani wrote: > Let's make this <code>true</code>. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:218: * matching filter or null On 2018/08/31 13:01:34, Manish Jethani wrote: > Let's make this <code>null</code>. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:237: } On 2018/08/31 13:01:34, Manish Jethani wrote: > Let's leave a blank line after the class definition. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:270: * Lookup table of previous matchesAny results On 2018/08/31 13:01:35, Manish Jethani wrote: > Let's make this `{@link Matcher#matchesAny}` Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:423: } On 2018/08/31 13:01:34, Manish Jethani wrote: > Let's leave a blank line here. Done. https://codereview.adblockplus.org/29869571/diff/29869572/lib/matcher.js#newc... lib/matcher.js:427: * Shared CombinedMatcher instance that should usually be used. On 2018/08/31 13:01:35, Manish Jethani wrote: > Let's make this `{@link CombinedMatcher}` Done.
https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js#newc... lib/matcher.js:161: * Returns the keyword used for a filter, null for unknown filters. Let's make this `<code>null</code>` as well while we're at it. https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js#newc... lib/matcher.js:286: * @param {Filter} filter When we use @see, we're not being consistent in adding @param tags. Here we're adding them, but not for matchesAny for example. Let's just leave the @param tags out here when using @see. Also see my comment about using @implements in the long run. https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js#newc... lib/matcher.js:368: * @inheritdoc I don't think @inheritdoc makes sense here (also below), because CombinedMatches does not in fact extend Matcher. It does nothing for the generated documentation. In the long run we should probably consider adding a Matcher interface and the use @implements [1] to automatically inherit the documentation, but this would be separate thing. [1]: http://usejsdoc.org/tags-implements.html https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js#newc... lib/matcher.js:429: exports.defaultMatcher = new CombinedMatcher(); I think we should follow the convention of first declaring something and then exporting it. We're doing this at the end of lib/filterListener.js now. let defaultMatcher = new CombinedMatcher(); exports.defaultMatcher = defaultMatcher;
https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js#newc... lib/matcher.js:161: * Returns the keyword used for a filter, null for unknown filters. On 2018/09/03 18:46:49, Manish Jethani wrote: > Let's make this `<code>null</code>` as well while we're at it. Done. https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js#newc... lib/matcher.js:286: * @param {Filter} filter On 2018/09/03 18:46:49, Manish Jethani wrote: > When we use @see, we're not being consistent in adding @param tags. Here we're > adding them, but not for matchesAny for example. Let's just leave the @param > tags out here when using @see. > > Also see my comment about using @implements in the long run. The reason for this is that matchesAny does not have a filter parameter, so removing the @param tag causes eslint to complain since the definitions don't match. https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js#newc... lib/matcher.js:368: * @inheritdoc On 2018/09/03 18:46:49, Manish Jethani wrote: > I don't think @inheritdoc makes sense here (also below), because CombinedMatches > does not in fact extend Matcher. It does nothing for the generated > documentation. > > In the long run we should probably consider adding a Matcher interface and the > use @implements [1] to automatically inherit the documentation, but this would > be separate thing. > > [1]: http://usejsdoc.org/tags-implements.html Similarly to above, eslint complains here without @inherit doc https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js#newc... lib/matcher.js:429: exports.defaultMatcher = new CombinedMatcher(); On 2018/09/03 18:46:49, Manish Jethani wrote: > I think we should follow the convention of first declaring something and then > exporting it. We're doing this at the end of lib/filterListener.js now. > > let defaultMatcher = new CombinedMatcher(); > > exports.defaultMatcher = defaultMatcher; Done.
LGTM https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29872557/lib/matcher.js#newc... lib/matcher.js:286: * @param {Filter} filter On 2018/09/05 14:05:27, Jon Sonesen wrote: > On 2018/09/03 18:46:49, Manish Jethani wrote: > > When we use @see, we're not being consistent in adding @param tags. Here we're > > adding them, but not for matchesAny for example. Let's just leave the @param > > tags out here when using @see. > > > > Also see my comment about using @implements in the long run. > > The reason for this is that matchesAny does not have a filter parameter, so > removing the @param tag causes eslint to complain since the definitions don't > match. Oh damn. OK, no problem then.
https://codereview.adblockplus.org/29869571/diff/29875577/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29875577/lib/matcher.js#newc... lib/matcher.js:421: let defaultMatcher = new CombinedMatcher(); Nit: Hey, can we also add a blank line here? I didn't realize it was missing.
https://codereview.adblockplus.org/29869571/diff/29875577/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29875577/lib/matcher.js#newc... lib/matcher.js:421: let defaultMatcher = new CombinedMatcher(); On 2018/09/05 19:34:47, Manish Jethani wrote: > Nit: Hey, can we also add a blank line here? I didn't realize it was missing. Done.
https://codereview.adblockplus.org/29869571/diff/29875577/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29875577/lib/matcher.js#newc... lib/matcher.js:421: let defaultMatcher = new CombinedMatcher(); On 2018/09/07 03:06:44, Jon Sonesen wrote: > On 2018/09/05 19:34:47, Manish Jethani wrote: > > Nit: Hey, can we also add a blank line here? I didn't realize it was missing. > > Done. Looks like you forgot to upload the patch?
https://codereview.adblockplus.org/29869571/diff/29875577/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29869571/diff/29875577/lib/matcher.js#newc... lib/matcher.js:421: let defaultMatcher = new CombinedMatcher(); On 2018/09/07 03:14:59, Manish Jethani wrote: > On 2018/09/07 03:06:44, Jon Sonesen wrote: > > On 2018/09/05 19:34:47, Manish Jethani wrote: > > > Nit: Hey, can we also add a blank line here? I didn't realize it was > missing. > > > > Done. > > Looks like you forgot to upload the patch? Sorry, I just saw your email. Can you also upload the latest patch here? It's just a formality. Nevertheless, LGTM.
Done, thanks!
Done, thanks! 
|
