Skip to content

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
@ktbytechibong ktbytechibong requested a review from cpcallen 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
Collaborator

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

@cpcallen
Copy link
Collaborator

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

@cpcallen cpcallen requested a review from maribethb August 20, 2022 02:14
@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
Contributor

@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 RaspberryPiFoundation: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.

4 participants