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

Remove the SinglePipeDefinition Syntax #487

Merged
merged 4 commits into from
Dec 18, 2023

Conversation

georg-schwarz
Copy link
Member

Right now, we have two syntaxes to do the same thing.
I feel this leads to unnecessary complexity.
I suggest to remove the verbose SinglePipeDefinition syntax in favor of the ChainedPipeDefinition syntax using arrows.

Comment on lines 20 to 18
export class PipeWrapper<N extends PipeDefinition = PipeDefinition>
implements AstNodeWrapper<N>
export class PipeWrapper
implements AstNodeWrapper<PipeDefinition | BlocktypePipeline>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove the generic? astNode can be narrowed down with the generic.

export class PipeWrapper<T extends PipeDefinition | BlocktypePipeline>
 implements AstNodeWrapper<T> { ... }

Current State:

declare const pipeline: BlocktypePipeline;
const wrapper = new PipeWrapper(pipeline, 29);
const node = wrapper.astNode; // <- typed as 'PipeDefinition | BlocktypePipeline' instead of 'BlocktypePipeline'

@joluj
Copy link
Contributor

joluj commented Dec 18, 2023

@georg-schwarz

  • I had to add a default type to prevent having to update the type in several places.
  • I had to remove the improved typing of getToDiagnostic and getFromDiagnostic since the types went wild in ValidationContext and langium's ValidationAcceptor

@georg-schwarz
Copy link
Member Author

Awesome! Thx for cleaning up my mess @joluj :-)

@georg-schwarz georg-schwarz merged commit f61aaf7 into main Dec 18, 2023
2 checks passed
@georg-schwarz georg-schwarz deleted the remove-single-pipe-definition-syntax branch December 18, 2023 16:34
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 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.

3 participants