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 Bazel packages with a central lint script #5790

Merged
merged 18 commits into from
Nov 2, 2021

Conversation

mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented Oct 28, 2021

Lint Bazel-built packages with a central yarn lint script at the root of the repository. This change also removes several cloudbuild files that are currently only used for linting.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@google-cla google-cla bot added the cla: yes label Oct 28, 2021
@mattsoulanille mattsoulanille marked this pull request as ready for review October 29, 2021 00:18
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @mattsoulanille)


BAZEL_MIGRATION.md, line 407 at r2 (raw file):

### Move linting to the repo-wide lint script
#### Add the new Bazel package to the [repo-wide tslint tsconfig](tsconfig_tslint.json):

how about the tsconfig.json file at the root directory, will that need to be updated?


scripts/package_dependencies.json, line 3 at r2 (raw file):

{
  "tfjs-automl": [],
  "tfjs-converter": [],

why the deps config needs to be updated?


tfjs-core/src/log.ts, line 18 at r2 (raw file):

 */

// tslint:disable-next-line:no-circular-imports

this is fixed by Jing here #5783, maybe you can try to sync again.

Copy link
Member Author

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)


BAZEL_MIGRATION.md, line 407 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

how about the tsconfig.json file at the root directory, will that need to be updated?

The normal tsconfig.json file does not need to be updated when tsconfig_tslint.json is updated. The tsconfig_tslint.json file is only used by tslint. It tells tslint the project structure that is contained in the Bazel BUILD files so it can correctly check rules that depend on types across our packages (we do this differently in google3, but I think it would be difficult to dreplicate).


scripts/package_dependencies.json, line 3 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

why the deps config needs to be updated?

I removed several cloudbuild.yml files from packages that were only running lint scripts. With this change, those packages are linted by the global lint script, so they had no more cloudbuild steps to run. package_dependencies.json exists only to track dependencies for the for the cloudbuild generator script, so those packages that no longer have a cloudbuild.yml file should be removed from it.


tfjs-core/src/log.ts, line 18 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

this is fixed by Jing here #5783, maybe you can try to sync again.

Thanks! There seems to be another one caused by #5784. base_side_effects.ts <-> platform_node.ts. I removed it and the tests still pass.

@pyu10055 pyu10055 merged commit 4415371 into tensorflow:master Nov 2, 2021
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.

2 participants