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

feat: procedure blocks update models #6672

Merged

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Nov 30, 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

Work on #6526

Proposed Changes

Adds logic to the procedure blocks to update their data models when they are composed.

Reason for Changes

Actually get procedure blocks to use the source-of-truth data models.

Test Coverage

Enables tests for updating the data models.

Documentation

N/A

Additional Information

Dependent on #6671

@BeksOmega BeksOmega requested a review from a team as a code owner November 30, 2022 17:21
@BeksOmega BeksOmega requested a review from gonfunko November 30, 2022 17:21
@github-actions github-actions bot added the PR: feature Adds a feature label Nov 30, 2022
@BeksOmega BeksOmega force-pushed the feat/procedure-blocks-update-models branch from 15764bf to b5e3d02 Compare December 1, 2022 19:42
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Dec 1, 2022
@BeksOmega BeksOmega force-pushed the feat/procedure-blocks-update-models branch 2 times, most recently from d1653fe to 36c5c76 Compare December 7, 2022 00:53
@BeksOmega BeksOmega marked this pull request as draft December 7, 2022 00:54
@BeksOmega BeksOmega force-pushed the feat/procedure-blocks-update-models branch 2 times, most recently from dec458a to c4bc944 Compare December 14, 2022 22:30
@BeksOmega BeksOmega force-pushed the feat/procedure-blocks-update-models branch from 1db2b8d to 08b59bf Compare December 20, 2022 17:46
@BeksOmega BeksOmega force-pushed the feat/procedure-blocks-update-models branch from 08b59bf to 63f9f23 Compare January 4, 2023 16:46
@BeksOmega BeksOmega marked this pull request as ready for review January 4, 2023 16:54
Comment on lines 487 to 490
const sourceBlock = this.getSourceBlock();
const legalName = Procedures.findLegalName(newName.trim(), sourceBlock);
sourceBlock.model.setName(legalName);
return legalName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Procedures.rename appears to already be finding and returning a legal name; additionally, it seems like setting the name on the model should be done by Procedures.rename in case it gets called through some other mechanism. Would it make sense to move that line into Procedures.rename() and revert this to just calling Procedures.rename directly like it was?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call! When I was writing this I wasn't sure if we wanted everything to be 100% backwards compatible or not. If not, I was planning on removing rename so I duplicated the logic. But given we do want it to be backwards compatible, this is unnecessary.

Skipped one test that was breaking because of the isProcedureBlock check I added. Will re-enable in the next PR once procedure definitions have a doProcedureUpdate method.

tests/mocha/blocks/procedures_test.js Outdated Show resolved Hide resolved
tests/mocha/blocks/procedures_test.js Show resolved Hide resolved
@BeksOmega BeksOmega force-pushed the feat/procedure-blocks-update-models branch from ee618da to 616b4be Compare January 5, 2023 21:02
@BeksOmega BeksOmega merged commit 5d20b62 into google:develop Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants