Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getValueForExpr: add support for arrays #10627

Merged

Conversation

gilbox
Copy link
Contributor

@gilbox gilbox commented Jul 25, 2017

getValueForExpr: add support for arrays

Solves #10613 by adding support for array indexes for the items attribute of amp-list.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@gilbox gilbox force-pushed the gil--getValueForExpr-support-array-index branch from 0c06519 to e9e471b Compare July 25, 2017 17:57
@googlebot
Copy link

CLAs look good, thanks!

@robinpokorny
Copy link
Member

I find this unnecesary complicated, it would be sufficient to use (maybe better formated and/or with variables):

- if (!isObject(value) ||
+ if (!(isObject(value) || isArray(value))||
            value[part] === undefined ||
            !hasOwnProperty(value, part)) {
      value = undefined;
      break;
    }

The flow is harder to follow with the added continues. And arrayHasKey feels really hacky: hasOwnProperty should replace that magic in the first part of it and the second part needs no typeof.

src/json.js Outdated
}
value = value[part];
if (isObject(value) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could likely just remove this in favor of a truthy check:

if (value && value[part] !== undefined && hasOwnProperty(value, part)) {
  //...
}

Copy link
Contributor Author

@gilbox gilbox Jul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great feedback y'all! I've added a commit that I hope addresses all of your comments

@cschubiner
Copy link

Exciting stuff, thanks @gilbox!

@ampprojectbot ampprojectbot added this to the Pending Triage milestone Sep 12, 2017
@ampprojectbot
Copy link
Member

This issue seems to be in Pending Triage for awhile. Please triage this to an appropriate milestone.

@jridgewell jridgewell merged commit a9a33f8 into ampproject:master Dec 20, 2017
@jridgewell
Copy link
Contributor

Apologies for letting this fall through the cracks.

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
* getValueForExpr: add support for arrays

* code review: simplify

* lint 🐒
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants