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: add basic observable implementations of procedure interfaces #6489

Merged

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Oct 5, 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

Fixes #6514

Dependent on #6488

Proposed Changes

Adds some basic implementations of the procedure interfaces. They don't do any of the important stuff yet (i.e. triggering doProcedureUpdate or firing events).

Reason for Changes

Interfaces need to be implemented.

Test Coverage

N/A

Documentation

N/A

Additional Information

N/A

@BeksOmega BeksOmega requested a review from a team as a code owner October 5, 2022 21:52
@BeksOmega BeksOmega requested a review from cpcallen October 5, 2022 21:52
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.

A preliminary review pending discussion about #6488.

core/procedures/observable_parameter_model.ts Outdated Show resolved Hide resolved
core/procedures/observable_procedure_model.ts Outdated Show resolved Hide resolved
@BeksOmega BeksOmega force-pushed the feat/procedure-models-implementions branch from 2a6b864 to 76583a0 Compare October 10, 2022 20:08
@github-actions github-actions bot added the PR: feature Adds a feature label Oct 10, 2022
@BeksOmega BeksOmega force-pushed the feat/procedure-models-implementions branch 3 times, most recently from 09a2dae to b3dc7bb Compare October 13, 2022 17:21
@BeksOmega BeksOmega force-pushed the feat/procedure-models-implementions branch from 3d1748e to d44b501 Compare October 13, 2022 20:12
core/procedures/observable_procedure_model.ts Outdated Show resolved Hide resolved
@BeksOmega BeksOmega merged commit a7247af into google:develop Oct 13, 2022
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 found this tab spinning, with my approval from yesterday apparently unsubmitted. Grr...

constructor(
private readonly workspace: Workspace, name: string, id?: string) {
this.id = id ?? genUid();
this.variable = workspace.createVariable(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this try .getVariable(name) first, like setName does? And is there a way of doing that by calling setName—i.e., can tsc infer (or be made to infer) that setName does the initialisation on behalf of the constructor?

(Aside: the latter is the sort of thing I bet Ezno can handle no problem, no special annotation required…)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will look into this.

core/procedures/observable_parameter_model.ts Show resolved Hide resolved
core/procedures/observable_parameter_model.ts Show resolved Hide resolved
core/procedures/observable_procedure_map.ts Show resolved Hide resolved
core/procedures/observable_procedure_map.ts Show resolved Hide resolved
@BeksOmega BeksOmega deleted the feat/procedure-models-implementions 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.

Create basic concrete implementations of the procedure data models
3 participants