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 callers respond to models #6718

Merged

Conversation

BeksOmega
Copy link
Collaborator

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

Refactors the procedure caller blocks to rely on the backing data models.

Reason for Changes

Using the data models makes it so that procedures are explicitly defined!

Test Coverage

Enabled some tests, added some tests, reorganized some tests etc.

Documentation

N/A

Additional Information

Almost certainly best to review this commit-wise haha.

Dependent on #6706

@BeksOmega BeksOmega requested a review from a team as a code owner December 19, 2022 19:38
@BeksOmega BeksOmega requested a review from NeilFraser December 19, 2022 19:38
@github-actions github-actions bot added the PR: feature Adds a feature label Dec 19, 2022
@BeksOmega BeksOmega marked this pull request as draft December 19, 2022 19:39
@BeksOmega BeksOmega force-pushed the feat/procedure-callers-respond-to-models branch from 1f9163b to c53a4a1 Compare December 20, 2022 17:49
@BeksOmega BeksOmega force-pushed the feat/procedure-callers-respond-to-models branch from c53a4a1 to 4e79d35 Compare January 6, 2023 00:01
@BeksOmega BeksOmega marked this pull request as ready for review January 6, 2023 00:01
@BeksOmega BeksOmega force-pushed the feat/procedure-callers-respond-to-models branch from 346e097 to 8c688b4 Compare January 6, 2023 16:40
tests/mocha/blocks/procedures_test.js Outdated Show resolved Hide resolved
@BeksOmega BeksOmega merged commit e1995ae into google:develop Jan 6, 2023
@BeksOmega BeksOmega mentioned this pull request Jan 6, 2023
4 tasks
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.

I have been investigating unexpected diagnostics appearing in npm test output. Several were traced to two minor errors in this PR.


/**
* @param {string} name The name of the procedure model to find.
* @param {string[]} params The param names of the procedure model to find.
Copy link
Contributor

Choose a reason for hiding this comment

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

string[] is not valid Closure type syntax. Do you mean Array<string>?

This provoked a warning from closure-make-deps.

* Creates a procedure definition block with the given name and params,
* and returns the procedure model associated with it.
* @param {string} name The name of the procedure to create.
* @param {string[]} params The names of the parameters to create.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@BeksOmega
Copy link
Collaborator Author

Can fix!

@BeksOmega BeksOmega mentioned this pull request Jan 12, 2023
4 tasks
@BeksOmega BeksOmega deleted the feat/procedure-callers-respond-to-models branch May 14, 2024 16:26
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.

3 participants