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

Cleanup services module #1273

Merged
merged 1 commit into from
Jan 5, 2016
Merged

Conversation

jridgewell
Copy link
Contributor

  • Don’t call getServices with a second parameter (it’s not expecting it)
  • Reorder conditionals to make code clearer

- Don’t call `getServices` with a second parameter (it’s not expecting
it)
- Reorder conditionals to make code clearer
@ampsauce
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 561d569
Build details: https://travis-ci.org/ampsauce/amphtml/builds/99502591

(Please note that this is a fully automated comment.)

if (s.obj) {
return s.promise = Promise.resolve(s.obj);
if (!s.promise) {
s.promise = Promise.resolve(s.obj);
Copy link
Member

Choose a reason for hiding this comment

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

Web S=should probably have an assert here that s.obj is actually an object.

@cramforce
Copy link
Member

LGTM

@cramforce cramforce added the LGTM label Jan 1, 2016
@cramforce
Copy link
Member

(aside: Please add "NEEDS REVIEW" label when sending PR).

jridgewell added a commit that referenced this pull request Jan 5, 2016
@jridgewell jridgewell merged commit cf10b09 into ampproject:master Jan 5, 2016
@jridgewell jridgewell deleted the service-cleanup branch January 5, 2016 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants