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

🏗 ♻️ Fix type errors in /build-system/tasks #33074

Merged
merged 9 commits into from
Mar 17, 2021

Conversation

rileyajones
Copy link
Contributor

The ongoing effort to add typing to the build-system directory has finally reached the largest directory "tasks". Due to the large size of this directory the e2e, performance, and visual-diff subdirectories have been excluded from this PR but will be fixed in another upcoming PR.

The total number of type errors within the /build-system/tasks directory was 544 which combined with the additional 6 outstanding summed to 550.
After this PR there will be a total of 281 and without the e2e, performance, and visual-diff directories the number drops to 15.

#28387

build-system/tasks/check-owners.js Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/compile/compile.js Outdated Show resolved Hide resolved
build-system/tasks/extension-generator/index.js Outdated Show resolved Hide resolved
build-system/tasks/report-test-status.js Outdated Show resolved Hide resolved
@@ -241,8 +241,8 @@ const findConfigBitCommits = (
tokens.pop();
}
return {
hash: tokens.shift(),
authorDate: tokens.shift(),
hash: /** @type {string} */ (tokens.shift()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike my the other comment I left about string.split.shift this could technically be undefined but I trust that the @alanorozco understands this code better than I.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I had this missed this question!

This seems fine per format received: %h %aI %s, though we have the conditional .pop() above. I'd say this is good but I'll make a separate change to put up a guard before this cast.

Copy link
Member

Choose a reason for hiding this comment

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

#33266 for safer parsing/testing

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 @alanorozco

build-system/test-configs/dep-check-config.js Show resolved Hide resolved
build-system/tsconfig.json Outdated Show resolved Hide resolved
build-system/tsconfig.json Outdated Show resolved Hide resolved
@rileyajones rileyajones marked this pull request as ready for review March 4, 2021 21:02
@amp-owners-bot
Copy link

amp-owners-bot bot commented Mar 4, 2021

Hey @erwinmombay, @jridgewell! These files were changed:

build-system/babel-plugins/babel-plugin-transform-prune-namespace/index.js
build-system/babel-plugins/babel-plugin-transform-stringish-literals/index.js
build-system/eslint-rules/dict-string-keys.js
build-system/eslint-rules/html-template.js
build-system/eslint-rules/no-import-rename.js
build-system/eslint-rules/preact.js
build-system/eslint-rules/prefer-destructuring.js
build-system/eslint-rules/query-selector.js
build-system/eslint-rules/unused-private-field.js

Hey @rsimha! These files were changed:

build-system/compile/compile.js
build-system/pr-check/bundle-size.js
build-system/pr-check/checks.js
build-system/pr-check/cross-browser-tests.js
build-system/pr-check/e2e-tests.js
build-system/pr-check/experiment-build.js
build-system/pr-check/experiment-tests.js
build-system/pr-check/module-build.js
build-system/pr-check/module-tests.js
build-system/pr-check/nomodule-build.js
build-system/pr-check/nomodule-tests.js
build-system/pr-check/performance-tests.js
+6 more

Hey @alanorozco! These files were changed:

build-system/server/app-index/api/api.js
build-system/server/app-index/index.js
build-system/tasks/markdown-toc/index.js

Hey @estherkim! These files were changed:

build-system/tasks/e2e/describes-e2e.js
build-system/tasks/e2e/expect.js
build-system/tasks/e2e/repl.js

build-system/test-configs/karma.conf.js Show resolved Hide resolved
build-system/tasks/serve.js Show resolved Hide resolved
build-system/tasks/runtime-test/helpers.js Show resolved Hide resolved
build-system/tasks/performance/helpers.js Outdated Show resolved Hide resolved
build-system/tasks/get-zindex/index.js Show resolved Hide resolved
build-system/tasks/extension-generator/index.js Outdated Show resolved Hide resolved
build-system/pr-check/npm-checks.js Outdated Show resolved Hide resolved
build-system/tsconfig.json Outdated Show resolved Hide resolved
build-system/compile/compile.js Outdated Show resolved Hide resolved
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Lots of excellent changes in this PR! Some comments below.

build-system/compile/compile.js Outdated Show resolved Hide resolved
build-system/pr-check/npm-checks.js Outdated Show resolved Hide resolved
build-system/pr-check/utils.js Outdated Show resolved Hide resolved
build-system/tasks/check-owners.js Show resolved Hide resolved
build-system/tasks/report-test-status.js Outdated Show resolved Hide resolved
build-system/tasks/runtime-test/helpers.js Show resolved Hide resolved
build-system/tasks/runtime-test/runtime-test-base.js Outdated Show resolved Hide resolved
build-system/tasks/test-report-upload.js Outdated Show resolved Hide resolved
build-system/test-configs/karma.conf.js Show resolved Hide resolved
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Yay! LGTM from me after one important change to the visual diff file (bad merge).

Merge away after that's fixed and Daniel approves.

build-system/tasks/report-test-status.js Outdated Show resolved Hide resolved
build-system/tasks/visual-diff/index.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

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

Thanks & sorry for the delay! I have a hard timing telling when something is ready for re-review.

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.

None yet

6 participants