Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(141)

Issue 29803559: Issue 6741 - Use ES2015 classes in lib/filterClasses.js

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by Manish Jethani
Modified:
5 days, 21 hours ago
Reviewers:
kzar, sergei
CC:
Sebastian Noack
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+370 lines, -332 lines) Patch
M lib/filterClasses.js View 10 chunks +370 lines, -332 lines 3 comments Download

Messages

Total messages: 4
Manish Jethani
1 week, 1 day ago (2018-06-09 12:51:37 UTC) #1
Manish Jethani
Patch Set 1 What do you think? The general pattern is: /** * Description of ...
1 week, 1 day ago (2018-06-09 13:03:32 UTC) #2
kzar
I mean sure, the new class syntax looks OK to me I guess. I'm not ...
5 days, 21 hours ago (2018-06-12 11:46:15 UTC) #3
Manish Jethani
5 days, 21 hours ago (2018-06-12 11:59:15 UTC) #4
On 2018/06/12 11:46:15, kzar wrote:
> I mean sure, the new class syntax looks OK to me I guess. I'm not too excited
> about switching to it, but I'm not against it either - assuming browser
> support's there and the rest of the team agrees.

Yeah, I'm not a big fan of ES2015 classes either, but the way we are doing it
currently clearly could use some improvement. Initially I thought we could just
use Object.assign [1], but this is actually cleaner and also better for
documentation (JSDoc now correctly identifies properties instead of incorrectly
assuming them to be "global"). Overall I think just declaring a "class" as an
actual class at the language level is just neater.

Anyway, so I thought everyone already agreed to this?

[1]: https://codereview.adblockplus.org/29746555/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5