Apply property shorthand syntax following ECMAScript 6 format #3471

muxin · 2016-06-06T22:01:34Z

From previous PR
It might be better to apply property shorthand syntax according to ES6 format throughout the code base for consistency.

For example, converting

obj = { x: x, y: y };

to

obj = { x, y };

However, if that's the convention we want, I find it not that intuitive when it comes to cases like the following:

this.changeObservable_.fire({
  relayoutAll: relayoutAll,
  top: scrollTop,
  left: scrollLeft,
  width: size.width,
  height: size.height,
  velocity: velocity,
});

if I change the above block to

this.changeObservable_.fire({
  relayoutAll,
  top: scrollTop,
  left: scrollLeft,
  width: size.width,
  height: size.height,
  velocity,
});

I think it introduced un-intuitiveness here.

What do you think about it? @cramforce @dvoytenko @mkhatib

mkhatib · 2016-06-06T22:18:02Z

Yeah, agreed mixed usage of the shorthand and non-shorthand in the same object literal might be very off-setting. One thing we can do to reduce that is to either:

Only use short-hand when all of the properties can be shorthanded OR
Allow mixing BUT put all short-handed properties at the top of the object literal, like the following.

this.changeObservable_.fire({
  relayoutAll,
  velocity,
  top: scrollTop,
  left: scrollLeft,
  width: size.width,
  height: size.height,
});

jridgewell · 2016-06-06T22:34:00Z

I like option 2.

cramforce · 2016-06-07T00:12:21Z

Lets pick the one option our linter can already enforce.
@erwinmombay Is there something we can use?

muxin · 2016-06-07T00:27:59Z

I found a lint rule here. However it seems to be enforcing every instance no matter whether it mixes with non-shorthand properties or not.
On the eslint page, it said that

Others may find the terseness of the shorthand syntax harder to read and may not want to encourage it with this rule.

I don't have a strong opinion here. But enforcing the shorthand property syntax could be a pain for readability and also I am having some compiling errors from the closure compiler(looking into it). I wonder what do you guys think?

cramforce · 2016-06-07T00:30:54Z

I have little opinion. But not something worth spending more than a couple
hours on.

On Mon, Jun 6, 2016 at 5:28 PM, Yuxi Chen notifications@github.com wrote:

I found a lint rule here http://eslint.org/docs/rules/object-shorthand.
However it seems to be enforcing every instance no matter whether it mixes
with non-shorthand properties or not.
On the eslint page, it said that

Others may find the terseness of the shorthand syntax harder to read and
may not want to encourage it with this rule.

I don't have a strong opinion here. But enforcing the shorthand property
syntax could be a pain for readability and also I am having some compiling
errors from the closure compiler(looking into it). I wonder what do you
guys think?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3471 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAFeT-MxYxgV4XnZyyJmLkidxvGnJ-nyks5qJLsQgaJpZM4IvXIb
.

erwinmombay · 2016-06-07T00:31:15Z

yep @cramforce , pointed @muxin to eslint rules

erwinmombay · 2016-06-07T00:32:04Z

@muxin can you send me a screenshot of the errors for closure compiler? thanks

muxin · 2016-06-07T00:36:49Z

@erwinmombay This is the link to the failed travis build
https://travis-ci.org/ampproject/amphtml/builds/135740123

cramforce · 2016-06-07T00:40:45Z

I'm currently working on upgrading our compiler version to the latest. Lets
not worry about this error until after that has landed.

On Mon, Jun 6, 2016 at 5:32 PM, erwin mombay notifications@github.com
wrote:

@muxin https://github.com/muxin can you send me a screenshot of the
errors for closure compiler? thanks

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3471 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAFeT65Njx9YZqRFVxcPDfXnEGzudttyks5qJLwFgaJpZM4IvXIb
.

muxin · 2016-06-07T00:52:31Z

The closure compiler complains when I am changing

export const Util = {
  filter: filter,
  guid: guid,
  log: log,
  make: make,
  set: set,
};

to

export const Util = {filter, guid, log, make, set};

I don't know why that happens but if I revert the changes, the closure compiler would work fine.

muxin · 2016-06-07T00:58:20Z

FYI. I made the changes in this pull request by searching \b([a-zA-Z]+): \1\b using regular expression, and then manually left out object that is a mix.

muxin · 2016-06-08T18:10:40Z

.eslintrc

@@ -61,6 +61,7 @@
      "objectsInObjects": false,
      "arraysInObjects": false
    }],
+    "object-shorthand": [2, "properties"],


This rule will make the linter complain every time we use a non-shorthand syntax. Shall I make the lint level error or warning? Do I have to separate this change to another PR?

@erwinmombay What do you think about the error level? I found that travis would still fail if it sees lint warnings. So we might make it error level? Do you think I should make the lint rule a separate PR?

last conversation i think we had in channel, lets go with error. better for consistency. you can leave the change here just make sure to pull i the latest and run the linter again

OK sure, thanks for the confirmation.

erwinmombay · 2016-06-08T22:40:21Z

extensions/amp-carousel/0.1/test/test-slides.js

@@ -382,8 +382,8 @@ describe('Slides functional', () => {
        pos: 0.6,
        min: 0,
        max: 0,
-        prevTr: prevTr,
-        nextTr: nextTr,
+        prevTr,


so we're ok with the shorthand being anywhere? i thought from the conversation here that they should always be on top

I'll add another commit to rearrange the orders.

just want to clarify you are doing the work in this PR or another PR?

In the same PR, commit coming

On second thought, I think leaving the order as it is might be better. Sometimes we have similar objects of the same set of properties but different values like the following:

object1 { x: x, y: bla, z: z, } object2 { x: x, y: bla, z: null, }

I think converting it to

object1 { x, y: bla, z, } object2 { x, y: bla, z: null, }

makes more sense because object1 and object2 looks better if they have the same pattern.

@jridgewell @mkhatib @erwinmombay What do you think?

honestly i don't have any strong opinion on this. since we can't enforce it right now anyways i do think we should just leave it as is

erwinmombay · 2016-06-09T01:55:41Z

@muxin LGTM, please make sure to fetch the latest from master and run the linter again before merging this

cramforce · 2016-06-09T11:46:41Z

👍

muxin force-pushed the js-property-shorthand branch from 8feec99 to d5df69c Compare June 6, 2016 23:51

muxin force-pushed the js-property-shorthand branch 2 times, most recently from 9802010 to ff841a0 Compare June 8, 2016 18:06

muxin reviewed Jun 8, 2016
View reviewed changes

muxin changed the title ~~[WIP] Apply property shorthand syntax following ECMAScript 6 format~~ Apply property shorthand syntax following ECMAScript 6 format Jun 8, 2016

muxin added the NEEDS REVIEW label Jun 8, 2016

muxin force-pushed the js-property-shorthand branch from ff841a0 to c8514ad Compare June 8, 2016 21:22

muxin assigned erwinmombay Jun 8, 2016

erwinmombay reviewed Jun 8, 2016
View reviewed changes

erwinmombay added LGTM and removed NEEDS REVIEW labels Jun 9, 2016

muxin force-pushed the js-property-shorthand branch from c8514ad to ef5c45e Compare June 9, 2016 04:56

muxin added 3 commits June 9, 2016 11:09

Apply property shorthand syntax following ECMAScript 6 format

db8b234

Apply property shorthand syntax for mixed objects

dd4b1bd

Add linter rule for property shorthand

fd52bff

muxin force-pushed the js-property-shorthand branch from ef5c45e to fd52bff Compare June 9, 2016 18:14

muxin merged commit b1c21b2 into ampproject:master Jun 9, 2016

muxin deleted the js-property-shorthand branch June 9, 2016 22:23

rudygalfi mentioned this pull request Jun 15, 2016

Apply property shorthand syntax following ECMAScript 6 format #3504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply property shorthand syntax following ECMAScript 6 format #3471

Apply property shorthand syntax following ECMAScript 6 format #3471

muxin commented Jun 6, 2016 •

edited

mkhatib commented Jun 6, 2016

jridgewell commented Jun 6, 2016

cramforce commented Jun 7, 2016

muxin commented Jun 7, 2016

cramforce commented Jun 7, 2016

erwinmombay commented Jun 7, 2016

erwinmombay commented Jun 7, 2016

muxin commented Jun 7, 2016

cramforce commented Jun 7, 2016

muxin commented Jun 7, 2016 •

edited

muxin commented Jun 7, 2016

muxin Jun 8, 2016

muxin Jun 9, 2016

erwinmombay Jun 9, 2016

muxin Jun 9, 2016

erwinmombay Jun 8, 2016

muxin Jun 9, 2016

erwinmombay Jun 9, 2016

muxin Jun 9, 2016

muxin Jun 9, 2016

erwinmombay Jun 9, 2016

erwinmombay commented Jun 9, 2016

cramforce commented Jun 9, 2016

Apply property shorthand syntax following ECMAScript 6 format #3471

Apply property shorthand syntax following ECMAScript 6 format #3471

Conversation

muxin commented Jun 6, 2016 • edited

mkhatib commented Jun 6, 2016

jridgewell commented Jun 6, 2016

cramforce commented Jun 7, 2016

muxin commented Jun 7, 2016

cramforce commented Jun 7, 2016

erwinmombay commented Jun 7, 2016

erwinmombay commented Jun 7, 2016

muxin commented Jun 7, 2016

cramforce commented Jun 7, 2016

muxin commented Jun 7, 2016 • edited

muxin commented Jun 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Jun 9, 2016

cramforce commented Jun 9, 2016

muxin commented Jun 6, 2016 •

edited

muxin commented Jun 7, 2016 •

edited