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

Refactoring: Define Blocktypes solely in Code #445

Merged
merged 45 commits into from
Nov 2, 2023

Conversation

georg-schwarz
Copy link
Member

@georg-schwarz georg-schwarz commented Sep 4, 2023

This PR removes the need for language extensions - the execution extensions are not touched. Therfore, the BlockMetaInformation becomes more of a wrapper class for the AST elements.

This will also resolve #435 as soon as we do the same for constraints.

Next steps

  • do the same for Constraints.
  • move constraint evalution from execution to the language-server to allow their usage for blocktype properties.

Copy link
Member Author

@georg-schwarz georg-schwarz left a comment

Choose a reason for hiding this comment

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

I started this PR today and ran into some dead ends. Especially expressing the validation logic in Jayvee itself is not there yet.. Any ideas how to approach it @rhazn ?

You can find the specific code snippets when looking at the jv blocktype definitions where I left a TODO, I marked two exemplary ones as conversation.

);
it('should provide matching block executors for builtin block types', async () => {
useStdExtension();
(await getAllBuiltinBlocktypes()).forEach(
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, I wasn't able to pull that out so that each blocktype has its own test case. Issue is the asynchronous nature of loading the builtin blocktypes. Requires some further digging I guess.

The function getAllBuildinBlocktypes might be a good candidate to pull out and add as util functionality to the interpreter-lib

Copy link
Member Author

Choose a reason for hiding this comment

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

Pulled out function to model-util.

const outputPort = blocktypeDefinition.outputs[0];
assert(outputPort !== undefined);
this.outputType = getIOType(outputPort);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Design decision: I did not extract docs here since that would require the JayveeServices to be injected. However, we don't have it available at all places where we construct this class. Would be too big of a refactoring, so I chose to leave that out and extract the docs from comments in the docs generator instead. Might influence the hovering support of Jayvee.

@@ -15,21 +24,90 @@ interface BlockDocs {
examples?: ExampleDoc[];
}

export abstract class BlockMetaInformation extends MetaInformation {
export class BlockMetaInformation extends MetaInformation {
Copy link
Member Author

Choose a reason for hiding this comment

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

This class basically became an AST wrapper (like Valuetype and others). I did not move it yet as I'd like to have the MetaInformation stuff cohesively in one place. Would move them as soon as we did the same for Constraints.

@georg-schwarz georg-schwarz marked this pull request as ready for review September 7, 2023 16:21
Comment on lines 16 to 18
:::info
This mechanism is currently under construction! We are moving from a code-first approach (using typescript classes to generate the Jayvee model of the blocktype) to a model-first approach (parsing the Jayvee model of the blocktype and creating ast wrappers).
:::
Copy link
Member Author

Choose a reason for hiding this comment

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

We should not use these info boxes!

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 48b883c

return;
}
if (BlockMetaInformation.canBeWrapped(blocktypeDefinition)) {
allBuiltinBlocktypes.push(
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: duplicate blocktype definition

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 7d98ae4

Copy link
Contributor

@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

Pair reviewed in meeting.

@georg-schwarz georg-schwarz merged commit 84775e9 into main Nov 2, 2023
2 checks passed
@georg-schwarz georg-schwarz deleted the block-meta-inf-in-code branch November 2, 2023 15:11
@github-actions github-actions bot locked and limited conversation to collaborators Nov 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] No repeated execution with interpreter-lib
2 participants