Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552173002:
ES6 Support: do review comments left on proposed ES6 style guide (Closed)
DescriptionES6 Support: do review comments left on proposed ES6 style guide
R=scottchen@chromium.org,dpapad@chromium.org
BUG=671426
NOTRY=true
Committed: https://crrev.com/814fb451892a2177a7093a26732e14d856b2015f
Cr-Commit-Position: refs/heads/master@{#436767}
Patch Set 1 #
Total comments: 15
Patch Set 2 : review #
Total comments: 4
Patch Set 3 : dpapad@ review #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 18 (9 generated)
Description was changed from ========== ES6 support: do review comments left on proposed ES6 style guide R=scottchen@chromium.org BUG=671426 ========== to ========== ES6 style: do review comments left on proposed ES6 style guide R=scottchen@chromium.org,dpapad@chromium.org BUG=671426 ==========
dbeam@chromium.org changed reviewers: + dpapad@chromium.org
alright, after this (and whenever you both are happy), let's send an email to chromium-dev@ and/or eng review to start this process up
https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md File docs/es6_chromium.md (left): https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#oldcod... docs/es6_chromium.md:106: >Some descriptions and Usage examples are from [kangax](https://kangax.github. btw, scottchen@, you can't break mid-word like this. it breaks the formatting. i understand how this is at odd with our 80 col wrapping rule :(
Description was changed from ========== ES6 style: do review comments left on proposed ES6 style guide R=scottchen@chromium.org,dpapad@chromium.org BUG=671426 ========== to ========== ES6 Support: do review comments left on proposed ES6 style guide R=scottchen@chromium.org,dpapad@chromium.org BUG=671426 ==========
https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md File docs/es6_chromium.md (right): https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcode65 docs/es6_chromium.md:65: // Add all the following siblings until it hits an <hr> Nit: Period missing at the end of the comment. https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcode80 docs/es6_chromium.md:80: document.querySelectorAll('hr').forEach(function(el) { Nit (optional): Can we use ES6 for this code? document.querySelectorAll('hr').forEach((el) => el.parentNode.removeChild(el)); https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:168: console.log(x); // 2 To make the two examples equivalent, the following line is missing. var x = 'foo'; // No error I still think that separating the runtime error to a separate example would be more clear. function f() { var a = 'hello'; var a = 'hola'; // No error! let b = 'world'; let b = 'mundo; // TypeError Identifier 'x' has already been declared. } https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:259: // Note: let Shape = class {...}; also works This comment is probably more meaningful if placed after line 266. https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:273: } Nit: New line between functions? https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:275: var PHI = (1 + Math.sqrt(5)) / 2; Can we use "let" in all ES6 examples? Or at least all ES6 examples appearing after the "let" example? https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:531: f(['bar', 42]); What happens if user calls it as follows? Can we add this to the example? f([1, 2, 3, 4]); Assuming extra array members are ignored? f([1, 2, 3, 4]); // 1, 2
https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md File docs/es6_chromium.md (right): https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcode65 docs/es6_chromium.md:65: // Add all the following siblings until it hits an <hr> On 2016/12/06 17:16:51, dpapad wrote: > Nit: Period missing at the end of the comment. Done. https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcode80 docs/es6_chromium.md:80: document.querySelectorAll('hr').forEach(function(el) { On 2016/12/06 17:16:52, dpapad wrote: > Nit (optional): Can we use ES6 for this code? > > document.querySelectorAll('hr').forEach((el) => el.parentNode.removeChild(el)); not until we've allowed it, no (see above where I reverted an => to function() {) https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:168: console.log(x); // 2 On 2016/12/06 17:16:51, dpapad wrote: > To make the two examples equivalent, the following line is missing. > var x = 'foo'; // No error > > I still think that separating the runtime error to a separate example would be > more clear. > > function f() { > var a = 'hello'; > var a = 'hola'; // No error! > > let b = 'world'; > let b = 'mundo; // TypeError Identifier 'x' has already been declared. > } Done. https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:259: // Note: let Shape = class {...}; also works On 2016/12/06 17:16:51, dpapad wrote: > This comment is probably more meaningful if placed after line 266. Done. https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:273: } On 2016/12/06 17:16:52, dpapad wrote: > Nit: New line between functions? Done. https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:275: var PHI = (1 + Math.sqrt(5)) / 2; On 2016/12/06 17:16:52, dpapad wrote: > Can we use "let" in all ES6 examples? Or at least all ES6 examples appearing > after the "let" example? i'm trying to use only 1 new concept at a time https://codereview.chromium.org/2552173002/diff/1/docs/es6_chromium.md#newcod... docs/es6_chromium.md:531: f(['bar', 42]); On 2016/12/06 17:16:52, dpapad wrote: > What happens if user calls it as follows? Can we add this to the example? > f([1, 2, 3, 4]); > > Assuming extra array members are ignored? > f([1, 2, 3, 4]); // 1, 2 Done.
Description was changed from ========== ES6 Support: do review comments left on proposed ES6 style guide R=scottchen@chromium.org,dpapad@chromium.org BUG=671426 ========== to ========== ES6 Support: do review comments left on proposed ES6 style guide R=scottchen@chromium.org,dpapad@chromium.org BUG=671426 NOTRY=true ==========
Message was sent while issue was closed.
LGTM with question. https://codereview.chromium.org/2552173002/diff/20001/docs/es6_chromium.md File docs/es6_chromium.md (right): https://codereview.chromium.org/2552173002/diff/20001/docs/es6_chromium.md#ne... docs/es6_chromium.md:187: let b = 'mundo; // TypeError Identifier 'x' has already been declared. s/x/b https://codereview.chromium.org/2552173002/diff/20001/docs/es6_chromium.md#ne... docs/es6_chromium.md:231: Prefer arrow functions over the function keyword and over `f.bind(this)`. Is this comment suggesting that arrow should be preferred for top-level functions? For example function add(a, b) { return a + b; } vs (a, b) => { return a + b; } Not sure if this is more readable, and it does not have any tangible benefit (unlike the arrows vs bind case, which is pretty much indisputable).
https://codereview.chromium.org/2552173002/diff/20001/docs/es6_chromium.md File docs/es6_chromium.md (right): https://codereview.chromium.org/2552173002/diff/20001/docs/es6_chromium.md#ne... docs/es6_chromium.md:187: let b = 'mundo; // TypeError Identifier 'x' has already been declared. On 2016/12/06 22:03:04, dpapad wrote: > s/x/b Done. https://codereview.chromium.org/2552173002/diff/20001/docs/es6_chromium.md#ne... docs/es6_chromium.md:231: Prefer arrow functions over the function keyword and over `f.bind(this)`. On 2016/12/06 22:03:04, dpapad wrote: > Is this comment suggesting that arrow should be preferred for top-level > functions? For example > > function add(a, b) { return a + b; } > vs > (a, b) => { return a + b; } > > Not sure if this is more readable, and it does not have any tangible benefit > (unlike the arrows vs bind case, which is pretty much indisputable). no, and have removed that wording for now
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2552173002/#ps40001 (title: "dpapad@ review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481063144864970,
"parent_rev": "8ee65a245f44f96074cfda9cd2e9946b10cf469d", "commit_rev":
"eb29fd5f67d9c7c3bfb9f8276659015e8e719914"}
Message was sent while issue was closed.
Description was changed from ========== ES6 Support: do review comments left on proposed ES6 style guide R=scottchen@chromium.org,dpapad@chromium.org BUG=671426 NOTRY=true ========== to ========== ES6 Support: do review comments left on proposed ES6 style guide R=scottchen@chromium.org,dpapad@chromium.org BUG=671426 NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== ES6 Support: do review comments left on proposed ES6 style guide R=scottchen@chromium.org,dpapad@chromium.org BUG=671426 NOTRY=true ========== to ========== ES6 Support: do review comments left on proposed ES6 style guide R=scottchen@chromium.org,dpapad@chromium.org BUG=671426 NOTRY=true Committed: https://crrev.com/814fb451892a2177a7093a26732e14d856b2015f Cr-Commit-Position: refs/heads/master@{#436767} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/814fb451892a2177a7093a26732e14d856b2015f Cr-Commit-Position: refs/heads/master@{#436767} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
