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: TS errors on dependent projects with certain tsconfig settings (#6360) #6361

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

ktbytechibong
Copy link
Contributor

@ktbytechibong ktbytechibong commented Aug 19, 2022

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

resolves #6360

Proposed Changes

Fairly minor refactoring to bypass some typescript errors appearing on dependent projects with certain tsconfig settings.

Behavior Before Change

N/A. No behavioral changes.

Behavior After Change

N/A. No behavioral changes.

Reason for Changes

Test Coverage

Existing tests should cover my changes:

  • run_node_test.js covers Generator#workspaceToCode
  • toolbox_test.js has "parseToolbox" suite covers parseToolboxTree
  • block_test.js has "toString" suite to test Block#tostring

Ran npm run test and all existing tests passed.

@ktbytechibong ktbytechibong requested a review from a team as a code owner August 19, 2022 17:05
@google-cla
Copy link

google-cla bot commented Aug 19, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Hello and thank you for sending us this useful pull request; it is appreciated!

It mostly looks great, but there is one place where I am hoping you might be able to suggest a better choice of name, and another where I think there is an actual error in our type definitions (maybe several errors) that should be fixed.

core/field_registry.ts Outdated Show resolved Hide resolved
core/block.ts Outdated Show resolved Hide resolved
@cpcallen
Copy link
Contributor

Assigning to @maribethb for follow-up review as I will be on annual leave until 20th August.

@cpcallen cpcallen assigned maribethb and unassigned cpcallen Aug 20, 2022
@ktbytechibong
Copy link
Contributor Author

@btw17 commented that #6360 should be fixed with #6362. This PR will still clean up some code, but not nearly as useful after that fix is merged.

@ktbytechibong
Copy link
Contributor Author

Ok, addressed comments in the PR. Thanks.

Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! LGTM =) If/once CI passes I'll go ahead and merge this, and it will go into our next release!

@BeksOmega BeksOmega merged commit e58cf77 into google:develop Sep 6, 2022
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

4 participants