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.

Sign up for GitHub

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

Jump to bottom

Fix LGTM errors in extensions/ #12770

Merged
merged 2 commits into from Jan 10, 2018
Merged

Fix LGTM errors in extensions/ #12770

merged 2 commits into from Jan 10, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Jan 10, 2018

@rsimha rsimha added the Cleanup label Jan 10, 2018
@rsimha rsimha self-assigned this Jan 10, 2018
@rsimha rsimha requested a review from zhouyx January 10, 2018 20:47
@rsimha
Copy link
Contributor Author

rsimha commented Jan 10, 2018

/to @zhouyx

jridgewell
jridgewell requested changes Jan 10, 2018
View reviewed changes
extensions/amp-analytics/0.1/requests.js Outdated
@@ -167,7 +167,7 @@ export class RequestHandler {
} else {
preUrl = appendEncodedParamStringToUrl(preUrl, extraUrlParamsStr);
}
return baseUrlTemplatePromise.then(preUrl => {
return baseUrlTemplatePromise.then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug! These are not equivalent!

Copy link
Contributor Author

@rsimha rsimha Jan 10, 2018 •
edited

Choose a reason for hiding this comment

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

That's right, I believe this is a bug in the original code. The then() block should use the local variable preUrl and not a new argument with the same name. Adding @zhouyx, who introduced this in #11837:

See https://github.com/ampproject/amphtml/pull/11837/files#diff-b951eee683ce1cc97c5e22f9abd32f11R169

Also see https://lgtm.com/projects/g/ampproject/amphtml/snapshot/a4d5dedaa4349a630d64ac4ffa41ed08b90f7efe/files/extensions/amp-analytics/0.1/requests.js?sort=name&dir=ASC&mode=heatmap&excluded=false

Copy link
Contributor

@zhouyx zhouyx Jan 10, 2018 •
edited

Choose a reason for hiding this comment

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

Failed to remove unused code in #11837. Had #12776 to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. File reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jridgewell @zhouyx PTAL

@rsimha
Copy link
Contributor Author

rsimha commented Jan 10, 2018

Errors fixed in https://lgtm.com/projects/g/ampproject/amphtml/rev/pr-050402326607b01a74faf3499b47412f5fb68cac

@rsimha rsimha merged commit 56e5440 into ampproject:master Jan 10, 2018
@rsimha rsimha deleted the 2018-01-10-ExtensionsLgtm branch January 10, 2018 22:33
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
@rsimha @gzgogo
Fix LGTM errors in extensions/ (ampproject#12770)
7acf3fb
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
@rsimha @RanAbram
Fix LGTM errors in extensions/ (ampproject#12770)
bb1a92a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jridgewell jridgewell jridgewell approved these changes

@zhouyx zhouyx zhouyx approved these changes

Assignees

@rsimha rsimha

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rsimha @jridgewell @zhouyx @googlebot