Issue 29897555: Issue 6940 - Use underscore prefixes lib/matcher.js (Closed)
|
Created:
Oct. 1, 2018, 5:46 a.m. by Jon Sonesen Modified:
Oct. 24, 2018, 8:40 p.m. Reviewers:
Manish Jethani Visibility:
Public. |
DescriptionIssue 6940 - Use underscore prefixes lib/matcher.js
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase #
Total comments: 22
Patch Set 3 : Address some PS2 comments #Patch Set 4 : Rebase #Patch Set 5 : Do jsdoc #
Total comments: 6
Patch Set 6 : Address PS5 Comment #
Total comments: 2
Patch Set 7 : Address PS6 comment #MessagesTotal messages: 29
Pretty sure the commit message could be changed, since it seems this issue may include a commit or two and may still need discussion.
I think it would be more practical to base this on https://codereview.adblockplus.org/29892596/ What do you think?
On 2018/10/01 10:25:56, Manish Jethani wrote: > I think it would be more practical to base this on > https://codereview.adblockplus.org/29892596/ > > What do you think? Yeah you're right, guess I just wanted to go ahead and get a review opened, will rebase once that lands.
https://codereview.adblockplus.org/29897555/diff/29897556/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29897556/lib/matcher.js#newc... lib/matcher.js:44: this._keywordByFilter = new Map(); I think we should add the `@private` tag here. Other than this, the change looks good to me, but let's wait for the rebase.
https://codereview.adblockplus.org/29897555/diff/29897556/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29897556/lib/matcher.js#newc... lib/matcher.js:44: this._keywordByFilter = new Map(); On 2018/10/01 15:08:59, Manish Jethani wrote: > I think we should add the `@private` tag here. I'm wondering where exactly it should be added. I guess let's make it a practice of adding it last (after `@type` here).
You could rebase this one now.
https://codereview.adblockplus.org/29897555/diff/29897556/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29897556/lib/matcher.js#newc... lib/matcher.js:44: this._keywordByFilter = new Map(); On 2018/10/01 15:10:00, Manish Jethani wrote: > On 2018/10/01 15:08:59, Manish Jethani wrote: > > I think we should add the `@private` tag here. > > I'm wondering where exactly it should be added. I guess let's make it a practice > of adding it last (after `@type` here). Done.
https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:60: * @private I've already started using @private/@protected elsewhere [1], but I put it as the last thing in the comment. Could you please make @private the last line in the comment? [1]: https://hg.adblockplus.org/adblockpluscore/rev/4a0244a174e6#l2.57 https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:125: * @protected Same here.
https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:170: _checkEntryMatch(keyword, location, typeMask, docDomain, thirdParty, sitekey, About this function, I think we can make it "protected". Let's remove the underscore and add the @protected tag. https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:257: this.resultCache = new Map(); Let's do this entire file while we're at it. I don't think resultCache needs to be public. It can private (underscore and @private). https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:303: findKeyword(filter) If we're marking fineKeyword @protected in Matcher, let's do it here in CombinedMatcher as well. https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:317: matchesAnyInternal(location, typeMask, docDomain, thirdParty, sitekey, Let's make this private (underscore and @private). https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:347: * @inheritdoc There are two instances of @inheritdoc in this file, neither of them makes sense because there is no inheritance. Let's remove them.
https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:257: this.resultCache = new Map(); On 2018/10/21 12:52:04, Manish Jethani wrote: > Let's do this entire file while we're at it. I don't think resultCache needs to > be public. It can private (underscore and @private). FYI we're having a discussion [1] about making blacklist and whitelist private as well. I think it makes sense to make them private. [1]: https://codereview.adblockplus.org/29896562/
Yeah, I agree as well and it can easily be added to this patch set.
https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:60: * @private On 2018/10/21 12:12:19, Manish Jethani wrote: > I've already started using @private/@protected elsewhere [1], but I put it as > the last thing in the comment. Could you please make @private the last line in > the comment? > > [1]: https://hg.adblockplus.org/adblockpluscore/rev/4a0244a174e6#l2.57 Acknowledged. https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:125: * @protected On 2018/10/21 12:12:19, Manish Jethani wrote: > Same here. Acknowledged. https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:170: _checkEntryMatch(keyword, location, typeMask, docDomain, thirdParty, sitekey, On 2018/10/21 12:52:04, Manish Jethani wrote: > About this function, I think we can make it "protected". Let's remove the > underscore and add the @protected tag. Acknowledged. https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:257: this.resultCache = new Map(); On 2018/10/22 22:57:50, Manish Jethani wrote: > On 2018/10/21 12:52:04, Manish Jethani wrote: > > Let's do this entire file while we're at it. I don't think resultCache needs > to > > be public. It can private (underscore and @private). > > FYI we're having a discussion [1] about making blacklist and whitelist private > as well. I think it makes sense to make them private. > > [1]: https://codereview.adblockplus.org/29896562/ Acknowledged. In the new patch set, I made them private and realized they are in fact not private. in lib/subscriptionClasses.js the "defaultsMap" literal is used to fetch the filter groups (not a very good design choice as it seems everyone has thought they are private.) The same applies in lib/filterClassesjs and test/filterListener.js so my first reaction is to just make them protected. https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:303: findKeyword(filter) On 2018/10/21 12:52:03, Manish Jethani wrote: > If we're marking fineKeyword @protected in Matcher, let's do it here in > CombinedMatcher as well. Done. https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:317: matchesAnyInternal(location, typeMask, docDomain, thirdParty, sitekey, On 2018/10/21 12:52:03, Manish Jethani wrote: > Let's make this private (underscore and @private). Acknowledged. https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:347: * @inheritdoc On 2018/10/21 12:52:04, Manish Jethani wrote: > There are two instances of @inheritdoc in this file, neither of them makes sense > because there is no inheritance. Let's remove them. Doing this makes eslint throw errors for the valid-jsdoc plugin
See comments inline. Also, the patch no longer applies cleanly. Can you rebase on the next branch? Let's also change the commit message now to something like "Issue 6940 - Use underscore prefixes in lib/matcher.js" (or if you can think of something better) https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:257: this.resultCache = new Map(); On 2018/10/23 01:28:02, Jon Sonesen wrote: > On 2018/10/22 22:57:50, Manish Jethani wrote: > > On 2018/10/21 12:52:04, Manish Jethani wrote: > > > Let's do this entire file while we're at it. I don't think resultCache needs > > to > > > be public. It can private (underscore and @private). > > > > FYI we're having a discussion [1] about making blacklist and whitelist private > > as well. I think it makes sense to make them private. > > > > [1]: https://codereview.adblockplus.org/29896562/ > > Acknowledged. In the new patch set, I made them private and realized they are in > fact not private. in lib/subscriptionClasses.js the "defaultsMap" literal is > used to fetch the filter groups (not a very good design choice as it seems > everyone has thought they are private.) The same applies in lib/filterClassesjs > and test/filterListener.js so my first reaction is to just make them protected. I don't think that has anything to do with the change here. We can make these properties private. https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:347: * @inheritdoc On 2018/10/23 01:28:01, Jon Sonesen wrote: > On 2018/10/21 12:52:04, Manish Jethani wrote: > > There are two instances of @inheritdoc in this file, neither of them makes > sense > > because there is no inheritance. Let's remove them. > > Doing this makes eslint throw errors for the valid-jsdoc plugin Oh yeah, I think you had mentioned this earlier as well. I forgot. Welp, let's leave it then.
https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:257: this.resultCache = new Map(); On 2018/10/23 03:10:14, Manish Jethani wrote: > On 2018/10/23 01:28:02, Jon Sonesen wrote: > > On 2018/10/22 22:57:50, Manish Jethani wrote: > > > On 2018/10/21 12:52:04, Manish Jethani wrote: > > > > Let's do this entire file while we're at it. I don't think resultCache > needs > > > to > > > > be public. It can private (underscore and @private). > > > > > > FYI we're having a discussion [1] about making blacklist and whitelist > private > > > as well. I think it makes sense to make them private. > > > > > > [1]: https://codereview.adblockplus.org/29896562/ > > > > Acknowledged. In the new patch set, I made them private and realized they are > in > > fact not private. in lib/subscriptionClasses.js the "defaultsMap" literal is > > used to fetch the filter groups (not a very good design choice as it seems > > everyone has thought they are private.) The same applies in > lib/filterClassesjs > > and test/filterListener.js so my first reaction is to just make them > protected. > > I don't think that has anything to do with the change here. We can make these > properties private. It does, have you tested making them private? I can upload the patch set that only changes properties but it breaks *alot* of tests
https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:257: this.resultCache = new Map(); On 2018/10/23 03:16:42, Jon Sonesen wrote: > On 2018/10/23 03:10:14, Manish Jethani wrote: > > On 2018/10/23 01:28:02, Jon Sonesen wrote: > > > On 2018/10/22 22:57:50, Manish Jethani wrote: > > > > On 2018/10/21 12:52:04, Manish Jethani wrote: > > > > > Let's do this entire file while we're at it. I don't think resultCache > > needs > > > > to > > > > > be public. It can private (underscore and @private). > > > > > > > > FYI we're having a discussion [1] about making blacklist and whitelist > > private > > > > as well. I think it makes sense to make them private. > > > > > > > > [1]: https://codereview.adblockplus.org/29896562/ > > > > > > Acknowledged. In the new patch set, I made them private and realized they > are > > in > > > fact not private. in lib/subscriptionClasses.js the "defaultsMap" literal is > > > used to fetch the filter groups (not a very good design choice as it seems > > > everyone has thought they are private.) The same applies in > > lib/filterClassesjs > > > and test/filterListener.js so my first reaction is to just make them > > protected. > > > > I don't think that has anything to do with the change here. We can make these > > properties private. > > It does, have you tested making them private? I can upload the patch set that > only changes properties but it breaks *alot* of tests You'll have to modify the tests of course, so they use the underscored names of the properties. I think you have to change exactly one line for this.
On 2018/10/23 03:23:17, Manish Jethani wrote: > https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js > File lib/matcher.js (right): > > https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... > lib/matcher.js:257: this.resultCache = new Map(); > On 2018/10/23 03:16:42, Jon Sonesen wrote: > > On 2018/10/23 03:10:14, Manish Jethani wrote: > > > On 2018/10/23 01:28:02, Jon Sonesen wrote: > > > > On 2018/10/22 22:57:50, Manish Jethani wrote: > > > > > On 2018/10/21 12:52:04, Manish Jethani wrote: > > > > > > Let's do this entire file while we're at it. I don't think resultCache > > > needs > > > > > to > > > > > > be public. It can private (underscore and @private). > > > > > > > > > > FYI we're having a discussion [1] about making blacklist and whitelist > > > private > > > > > as well. I think it makes sense to make them private. > > > > > > > > > > [1]: https://codereview.adblockplus.org/29896562/ > > > > > > > > Acknowledged. In the new patch set, I made them private and realized they > > are > > > in > > > > fact not private. in lib/subscriptionClasses.js the "defaultsMap" literal > is > > > > used to fetch the filter groups (not a very good design choice as it seems > > > > everyone has thought they are private.) The same applies in > > > lib/filterClassesjs > > > > and test/filterListener.js so my first reaction is to just make them > > > protected. > > > > > > I don't think that has anything to do with the change here. We can make > these > > > properties private. > > > > It does, have you tested making them private? I can upload the patch set that > > only changes properties but it breaks *alot* of tests > > You'll have to modify the tests of course, so they use the underscored names of > the properties. I think you have to change exactly one line for this. No, every line which passes a black or white list must be changed and places as listed above. I know this because I got it working
https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:257: this.resultCache = new Map(); On 2018/10/23 03:23:17, Manish Jethani wrote: > On 2018/10/23 03:16:42, Jon Sonesen wrote: > > On 2018/10/23 03:10:14, Manish Jethani wrote: > > > On 2018/10/23 01:28:02, Jon Sonesen wrote: > > > > On 2018/10/22 22:57:50, Manish Jethani wrote: > > > > > On 2018/10/21 12:52:04, Manish Jethani wrote: > > > > > > Let's do this entire file while we're at it. I don't think resultCache > > > needs > > > > > to > > > > > > be public. It can private (underscore and @private). > > > > > > > > > > FYI we're having a discussion [1] about making blacklist and whitelist > > > private > > > > > as well. I think it makes sense to make them private. > > > > > > > > > > [1]: https://codereview.adblockplus.org/29896562/ > > > > > > > > Acknowledged. In the new patch set, I made them private and realized they > > are > > > in > > > > fact not private. in lib/subscriptionClasses.js the "defaultsMap" literal > is > > > > used to fetch the filter groups (not a very good design choice as it seems > > > > everyone has thought they are private.) The same applies in > > > lib/filterClassesjs > > > > and test/filterListener.js so my first reaction is to just make them > > > protected. > > > > > > I don't think that has anything to do with the change here. We can make > these > > > properties private. > > > > It does, have you tested making them private? I can upload the patch set that > > only changes properties but it breaks *alot* of tests > > You'll have to modify the tests of course, so they use the underscored names of > the properties. I think you have to change exactly one line for this. Please try just changing this line: https://hg.adblockplus.org/adblockpluscore/file/e8254db792d0/test/filterListe... To this: let matcher = defaultMatcher["_" + type];
On 2018/10/23 03:28:42, Manish Jethani wrote: > https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js > File lib/matcher.js (right): > > https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... > lib/matcher.js:257: this.resultCache = new Map(); > On 2018/10/23 03:23:17, Manish Jethani wrote: > > On 2018/10/23 03:16:42, Jon Sonesen wrote: > > > On 2018/10/23 03:10:14, Manish Jethani wrote: > > > > On 2018/10/23 01:28:02, Jon Sonesen wrote: > > > > > On 2018/10/22 22:57:50, Manish Jethani wrote: > > > > > > On 2018/10/21 12:52:04, Manish Jethani wrote: > > > > > > > Let's do this entire file while we're at it. I don't think > resultCache > > > > needs > > > > > > to > > > > > > > be public. It can private (underscore and @private). > > > > > > > > > > > > FYI we're having a discussion [1] about making blacklist and whitelist > > > > private > > > > > > as well. I think it makes sense to make them private. > > > > > > > > > > > > [1]: https://codereview.adblockplus.org/29896562/ > > > > > > > > > > Acknowledged. In the new patch set, I made them private and realized > they > > > are > > > > in > > > > > fact not private. in lib/subscriptionClasses.js the "defaultsMap" > literal > > is > > > > > used to fetch the filter groups (not a very good design choice as it > seems > > > > > everyone has thought they are private.) The same applies in > > > > lib/filterClassesjs > > > > > and test/filterListener.js so my first reaction is to just make them > > > > protected. > > > > > > > > I don't think that has anything to do with the change here. We can make > > these > > > > properties private. > > > > > > It does, have you tested making them private? I can upload the patch set > that > > > only changes properties but it breaks *alot* of tests > > > > You'll have to modify the tests of course, so they use the underscored names > of > > the properties. I think you have to change exactly one line for this. > > Please try just changing this line: > > https://hg.adblockplus.org/adblockpluscore/file/e8254db792d0/test/filterListe... > > To this: > > let matcher = defaultMatcher["_" + type]; Ha! Damn, my bad
On 2018/10/23 03:28:42, Manish Jethani wrote: > https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js > File lib/matcher.js (right): > > https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... > lib/matcher.js:257: this.resultCache = new Map(); > On 2018/10/23 03:23:17, Manish Jethani wrote: > > On 2018/10/23 03:16:42, Jon Sonesen wrote: > > > On 2018/10/23 03:10:14, Manish Jethani wrote: > > > > On 2018/10/23 01:28:02, Jon Sonesen wrote: > > > > > On 2018/10/22 22:57:50, Manish Jethani wrote: > > > > > > On 2018/10/21 12:52:04, Manish Jethani wrote: > > > > > > > Let's do this entire file while we're at it. I don't think > resultCache > > > > needs > > > > > > to > > > > > > > be public. It can private (underscore and @private). > > > > > > > > > > > > FYI we're having a discussion [1] about making blacklist and whitelist > > > > private > > > > > > as well. I think it makes sense to make them private. > > > > > > > > > > > > [1]: https://codereview.adblockplus.org/29896562/ > > > > > > > > > > Acknowledged. In the new patch set, I made them private and realized > they > > > are > > > > in > > > > > fact not private. in lib/subscriptionClasses.js the "defaultsMap" > literal > > is > > > > > used to fetch the filter groups (not a very good design choice as it > seems > > > > > everyone has thought they are private.) The same applies in > > > > lib/filterClassesjs > > > > > and test/filterListener.js so my first reaction is to just make them > > > > protected. > > > > > > > > I don't think that has anything to do with the change here. We can make > > these > > > > properties private. > > > > > > It does, have you tested making them private? I can upload the patch set > that > > > only changes properties but it breaks *alot* of tests > > > > You'll have to modify the tests of course, so they use the underscored names > of > > the properties. I think you have to change exactly one line for this. > > Please try just changing this line: > > https://hg.adblockplus.org/adblockpluscore/file/e8254db792d0/test/filterListe... > > To this: > > let matcher = defaultMatcher["_" + type]; Ha! Damn, my bad
https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:257: this.resultCache = new Map(); On 2018/10/23 03:28:42, Manish Jethani wrote: > On 2018/10/23 03:23:17, Manish Jethani wrote: > > On 2018/10/23 03:16:42, Jon Sonesen wrote: > > > On 2018/10/23 03:10:14, Manish Jethani wrote: > > > > On 2018/10/23 01:28:02, Jon Sonesen wrote: > > > > > On 2018/10/22 22:57:50, Manish Jethani wrote: > > > > > > On 2018/10/21 12:52:04, Manish Jethani wrote: > > > > > > > Let's do this entire file while we're at it. I don't think > resultCache > > > > needs > > > > > > to > > > > > > > be public. It can private (underscore and @private). > > > > > > > > > > > > FYI we're having a discussion [1] about making blacklist and whitelist > > > > private > > > > > > as well. I think it makes sense to make them private. > > > > > > > > > > > > [1]: https://codereview.adblockplus.org/29896562/ > > > > > > > > > > Acknowledged. In the new patch set, I made them private and realized > they > > > are > > > > in > > > > > fact not private. in lib/subscriptionClasses.js the "defaultsMap" > literal > > is > > > > > used to fetch the filter groups (not a very good design choice as it > seems > > > > > everyone has thought they are private.) The same applies in > > > > lib/filterClassesjs > > > > > and test/filterListener.js so my first reaction is to just make them > > > > protected. > > > > > > > > I don't think that has anything to do with the change here. We can make > > these > > > > properties private. > > > > > > It does, have you tested making them private? I can upload the patch set > that > > > only changes properties but it breaks *alot* of tests > > > > You'll have to modify the tests of course, so they use the underscored names > of > > the properties. I think you have to change exactly one line for this. > > Please try just changing this line: > > https://hg.adblockplus.org/adblockpluscore/file/e8254db792d0/test/filterListe... > > To this: > > let matcher = defaultMatcher["_" + type]; Yeah I am the worst, thanks for your help and patience
https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29917621/lib/matcher.js#newc... lib/matcher.js:257: this.resultCache = new Map(); On 2018/10/23 04:08:03, Jon Sonesen wrote: > On 2018/10/23 03:28:42, Manish Jethani wrote: > > On 2018/10/23 03:23:17, Manish Jethani wrote: > > > On 2018/10/23 03:16:42, Jon Sonesen wrote: > > > > On 2018/10/23 03:10:14, Manish Jethani wrote: > > > > > On 2018/10/23 01:28:02, Jon Sonesen wrote: > > > > > > On 2018/10/22 22:57:50, Manish Jethani wrote: > > > > > > > On 2018/10/21 12:52:04, Manish Jethani wrote: > > > > > > > > Let's do this entire file while we're at it. I don't think > > resultCache > > > > > needs > > > > > > > to > > > > > > > > be public. It can private (underscore and @private). > > > > > > > > > > > > > > FYI we're having a discussion [1] about making blacklist and > whitelist > > > > > private > > > > > > > as well. I think it makes sense to make them private. > > > > > > > > > > > > > > [1]: https://codereview.adblockplus.org/29896562/ > > > > > > > > > > > > Acknowledged. In the new patch set, I made them private and realized > > they > > > > are > > > > > in > > > > > > fact not private. in lib/subscriptionClasses.js the "defaultsMap" > > literal > > > is > > > > > > used to fetch the filter groups (not a very good design choice as it > > seems > > > > > > everyone has thought they are private.) The same applies in > > > > > lib/filterClassesjs > > > > > > and test/filterListener.js so my first reaction is to just make them > > > > > protected. > > > > > > > > > > I don't think that has anything to do with the change here. We can make > > > these > > > > > properties private. > > > > > > > > It does, have you tested making them private? I can upload the patch set > > that > > > > only changes properties but it breaks *alot* of tests > > > > > > You'll have to modify the tests of course, so they use the underscored names > > of > > > the properties. I think you have to change exactly one line for this. > > > > Please try just changing this line: > > > > > https://hg.adblockplus.org/adblockpluscore/file/e8254db792d0/test/filterListe... > > > > To this: > > > > let matcher = defaultMatcher["_" + type]; > > Yeah I am the worst, thanks for your help and patience Not at all, it happens to me too :) https://codereview.adblockplus.org/29897555/diff/29919600/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29919600/lib/matcher.js#newc... lib/matcher.js:226: docDomain, thirdParty, sitekey, The following lines are not properly aligned now. https://codereview.adblockplus.org/29897555/diff/29919600/lib/matcher.js#newc... lib/matcher.js:334: specificOnly) Not properly aligned. https://codereview.adblockplus.org/29897555/diff/29919600/lib/matcher.js#newc... lib/matcher.js:386: thirdParty, sitekey, specificOnly); Not properly aligned.
https://codereview.adblockplus.org/29897555/diff/29919600/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29919600/lib/matcher.js#newc... lib/matcher.js:226: docDomain, thirdParty, sitekey, On 2018/10/23 10:38:27, Manish Jethani wrote: > The following lines are not properly aligned now. Done. https://codereview.adblockplus.org/29897555/diff/29919600/lib/matcher.js#newc... lib/matcher.js:334: specificOnly) On 2018/10/23 10:38:27, Manish Jethani wrote: > Not properly aligned. Done. https://codereview.adblockplus.org/29897555/diff/29919600/lib/matcher.js#newc... lib/matcher.js:386: thirdParty, sitekey, specificOnly); On 2018/10/23 10:38:27, Manish Jethani wrote: > Not properly aligned. Done.
LGTM
Patch sent.
https://codereview.adblockplus.org/29897555/diff/29921564/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29921564/lib/matcher.js#newc... lib/matcher.js:182: specificOnly) Sorry, I missed this one. We need to fix the alignment here as well.
New patch set in a few minutes https://codereview.adblockplus.org/29897555/diff/29921564/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29897555/diff/29921564/lib/matcher.js#newc... lib/matcher.js:182: specificOnly) On 2018/10/24 18:43:54, Manish Jethani wrote: > Sorry, I missed this one. We need to fix the alignment here as well. oof, good catch
LGTM
On 2018/10/24 19:05:17, Manish Jethani wrote: > LGTM https://hg.adblockplus.org/adblockpluscore/rev/7126b8cc2fad You can close this now. 
|
