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

Lint remaining static enums in src #36821

Merged
merged 12 commits into from
Nov 15, 2021
Merged

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Nov 6, 2021

These require no refactoring, they're already fully static.

Partial for #36754

@amp-owners-bot
Copy link

amp-owners-bot bot commented Nov 6, 2021

Hey @erwinmombay! These files were changed:

build-system/babel-plugins/babel-plugin-transform-inline-isenumvalue/index.js
build-system/babel-plugins/babel-plugin-transform-inline-isenumvalue/test/fixtures/transform-assertions/isenumvalue/output.mjs
build-system/eslint-rules/camelcase.js
build-system/eslint-rules/enums.js

Hey @alanorozco! These files were changed:

extensions/amp-3q-player/0.1/amp-3q-player.js
extensions/amp-3q-player/0.1/test/test-amp-3q-player.js
extensions/amp-brid-player/0.1/amp-brid-player.js
extensions/amp-brid-player/0.1/test/test-amp-brid-player.js
extensions/amp-delight-player/0.1/amp-delight-player.js
extensions/amp-delight-player/0.1/test/test-amp-delight-player.js
extensions/amp-minute-media-player/0.1/amp-minute-media-player.js
extensions/amp-nexxtv-player/0.1/amp-nexxtv-player.js
extensions/amp-nexxtv-player/0.1/test/test-amp-nexxtv-player.js
extensions/amp-o2-player/0.1/amp-o2-player.js
extensions/amp-o2-player/0.1/test/test-amp-o2-player.js
extensions/amp-ooyala-player/0.1/amp-ooyala-player.js
+6 more

Hey @jeffkaufman! These files were changed:

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
extensions/amp-ad-network-doubleclick-impl/0.1/flexible-ad-slot-utils.js
extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js

Hey @gmajoulet! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.js
extensions/amp-story-education/0.1/amp-story-education.js
extensions/amp-story-interactive/0.1/amp-story-interactive-img-quiz.js
extensions/amp-story-interactive/0.1/amp-story-interactive-quiz.js
extensions/amp-story-interactive/0.1/interactive-disclaimer.js
extensions/amp-story-panning-media/0.1/amp-story-panning-media.js
extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/amp-story-shopping-config.js
extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-config.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-tag.js
+31 more

Hey @processprocess! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.js
extensions/amp-story-panning-media/0.1/amp-story-panning-media.js

Hey @t0mg! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.js

Hey @mszylkowski! These files were changed:

extensions/amp-story-interactive/0.1/amp-story-interactive-img-quiz.js
extensions/amp-story-interactive/0.1/amp-story-interactive-quiz.js
extensions/amp-story-interactive/0.1/interactive-disclaimer.js

Hey @newmuis! These files were changed:

extensions/amp-story-panning-media/0.1/amp-story-panning-media.js
extensions/amp-story/1.0/amp-story-access.js
extensions/amp-story/1.0/amp-story-base-layer.js
extensions/amp-story/1.0/amp-story-consent.js
extensions/amp-story/1.0/amp-story-draggable-drawer.js
extensions/amp-story/1.0/amp-story-embedded-component.js
extensions/amp-story/1.0/amp-story-form.js
extensions/amp-story/1.0/amp-story-hint.js
extensions/amp-story/1.0/amp-story-info-dialog.js
extensions/amp-story/1.0/amp-story-localization-service.js
extensions/amp-story/1.0/amp-story-open-page-attachment.js
extensions/amp-story/1.0/amp-story-page-attachment.js
+20 more

Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

Excited to see the results when this is through, and hope that my estimations have been wrong 🙃

@@ -1622,10 +1624,10 @@ export function setConsentStateForTesting(newConsentState) {
*
* Copied from src/video-interface.js.
*
* @const {!Object<string, string>}
Copy link
Member

Choose a reason for hiding this comment

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

surprised you found this given it didn't have an enum annotation

@@ -36,7 +36,9 @@
*
* @return {!Object}
*/
module.exports = function (context) {
// eslint-disable-next-line
module.exports = function create(context) {
Copy link
Member

@samouri samouri Nov 8, 2021

Choose a reason for hiding this comment

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

  1. Why do you need to disable line for the line?
  2. Was this PR automatically generated via the fixer for this rule? If yes how did you detect the vardecl had an enum annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Eslint requires you to have a meta: { fixable: 'code' } field if you have a fixer. I didn't feel like changing the full linter to satisfy.
    1. Yes
    2. What about vardecl?

@samouri
Copy link
Member

samouri commented Nov 8, 2021

Curious that v0 is down ~0.1kb

@jridgewell jridgewell force-pushed the static-enum-1 branch 2 times, most recently from 763785c to 352367f Compare November 12, 2021 05:12
@jridgewell jridgewell enabled auto-merge (squash) November 12, 2021 05:16
@jridgewell jridgewell force-pushed the static-enum-1 branch 4 times, most recently from 43e0129 to efbdc7d Compare November 13, 2021 09:07
@samouri samouri merged commit 556e94a into ampproject:main Nov 15, 2021
@jridgewell jridgewell deleted the static-enum-1 branch November 15, 2021 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants