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

Mute expected error: BLOCK_BY_CONSENT #15903

Merged
merged 3 commits into from Jun 8, 2018

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Jun 7, 2018

Fix #15871

src/error.js Outdated
@@ -550,7 +552,7 @@ export function resetAccumulatedErrorMessagesForTesting() {
* @visibleForTesting
*/
export function detectJsEngineFromStack() {
/** @constructor */
/** @class */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was trying to make linter happy. But type check doesn't like the change. I added // eslint-disable-line instead.

cc @rsimha

Copy link
Contributor

@rsimha rsimha Jun 8, 2018

Choose a reason for hiding this comment

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

If closure compiler prefers @constructor over @class, we can specify that preference here.

amphtml/.eslintrc

Lines 39 to 43 in cbf3929

"jsdoc": {
"tagNamePreference": {
"returns": "return",
"constant": "const"
},

There might be other places where we'd need to change the tag, but it should be a simple find replace, I'd imagine. You needn't include those fixes in this PR. They can be done separately.

Copy link
Contributor

@rsimha rsimha Jun 8, 2018

Choose a reason for hiding this comment

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

Correct, it does prefer @constructor: https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#constructor

You can add this line to .eslintrc under tagNamePreference:

        "class": "constructor",

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 for the investigation.

I've created #15941. Will have a separate PR to fix this.

@zhouyx
Copy link
Contributor Author

zhouyx commented Jun 8, 2018

PTAL : )

@zhouyx zhouyx merged commit 45df359 into ampproject:master Jun 8, 2018
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

4 participants