-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat: procedure blocks update models #6672
Conversation
15764bf
to
b5e3d02
Compare
d1653fe
to
36c5c76
Compare
dec458a
to
c4bc944
Compare
1db2b8d
to
08b59bf
Compare
08b59bf
to
63f9f23
Compare
blocks/procedures.js
Outdated
const sourceBlock = this.getSourceBlock(); | ||
const legalName = Procedures.findLegalName(newName.trim(), sourceBlock); | ||
sourceBlock.model.setName(legalName); | ||
return legalName; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
ee618da
to
616b4be
Compare
The basics
npm run format
andnpm 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