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

Yuhsuan/null check tsc silent #2344

Merged
merged 60 commits into from
Mar 28, 2024
Merged

Conversation

YuHsuan-Hwang
Copy link
Collaborator

@YuHsuan-Hwang YuHsuan-Hwang commented Feb 1, 2024

Description

This PR is for the first part of #2343. Files in /utilities, /models, and /services are migrated with the strictNullChecks option enabled.

A new script is added to trigger a separate ts compiler with the strcitNullChecks option enabled. tsc-silent is applied to suppress errors from files that are not migrated yet.

  • To run the null checks, run npm run strict-null-checks. This is also triggered before development and production builds so that developers and the CI are aware of the checks.
  • To include files for the null checks, add the files in the migratedFiles list in tsc-silent.config.js.

Checklist

For linked issues (if there are):

  • assignee and label added
  • ZenHub issue connection, board status, and estimate updated

For the pull request:

  • reviewers and assignee added
  • ZenHub estimate, milestone, and release (if needed) added
  • changelog updated / no changelog update needed
  • unit test added (for functions with no dependenies)
  • API documentation added (for public variables and methods in stores)

For dependencies:

  • e2e test passing / corresponding fix added / new e2e test created
  • protobuf version bumped / no protobuf version bumped needed
  • protobuf updated to the latest dev commit / no protobuf update needed
  • corresponding ICD test fix added (BackendService changed) / no ICD test fix needed (BackendService unchanged)
  • user manual prepared (for large new features)

@YuHsuan-Hwang
Copy link
Collaborator Author

Revised some fixes by changing the variable types instead of the values. Hope this reduces the possibility of regressions.

src/services/SplatalogueService.ts Show resolved Hide resolved
src/utilities/parsing/parsing.ts Show resolved Hide resolved
src/utilities/table/table.ts Outdated Show resolved Hide resolved
src/services/TileService.ts Outdated Show resolved Hide resolved
@kswang1029 kswang1029 added awaiting code changes For pull requests that require code changes and removed awaiting code review For pull requests that require code review labels Mar 5, 2024
@YuHsuan-Hwang
Copy link
Collaborator Author

@crocka Thanks for reviewing the changes! Please check my replies and revision.

@YuHsuan-Hwang YuHsuan-Hwang added awaiting code review For pull requests that require code review and removed awaiting code changes For pull requests that require code changes labels Mar 6, 2024
Copy link
Collaborator

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

No regression from e2e tests. With some other manual tests based on the changed files in the codebase, so far I do not see odd behaviors. Certainly my manual test coverage is not complete, but let's just proceed and see if there are issues from the tests in the future.

Copy link
Contributor

@acdo2002 acdo2002 left a comment

Choose a reason for hiding this comment

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

The BackendService modification has been applied to the MessangeController.ts in the ICD-RxJS repository. (ICD-RxJS branch mylin/modify_MessageController_IBackendResponse)
The ICD-RxJS normal tests and performances tests are all passed without any problem.

@YuHsuan-Hwang YuHsuan-Hwang removed the awaiting testing For pull requests that require testing label Mar 13, 2024
@YuHsuan-Hwang YuHsuan-Hwang added awaiting merge For pull requests ready for merge or pending backend/protobuf changes and removed awaiting code review For pull requests that require code review labels Mar 25, 2024
@YuHsuan-Hwang YuHsuan-Hwang removed the awaiting merge For pull requests ready for merge or pending backend/protobuf changes label Mar 28, 2024
@YuHsuan-Hwang YuHsuan-Hwang merged commit 1013f5b into dev Mar 28, 2024
6 checks passed
@YuHsuan-Hwang YuHsuan-Hwang deleted the yuhsuan/null_check_tsc_silent branch March 28, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: PR test and review
Development

Successfully merging this pull request may close these issues.

4 participants