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

Support async command execution #39

Merged
merged 3 commits into from
Feb 28, 2023
Merged

Support async command execution #39

merged 3 commits into from
Feb 28, 2023

Conversation

tortmayr
Copy link
Contributor

Change command API to support MaybePromise<void> instead of just plain voids. This enables async command execution e.g. for communicating with a modelserver
Contributed on behalf of STMicroelectronics

Change command API to support MaybePromise<void> instead of just plain voids. This enables async command execution e.g. for communicating with a modelserver
Contributed on behalf of STMicroelectronics
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Change looks good to me for the most part, thank you very much Tobias! I just have a question regarding the compound command.

@@ -108,12 +110,14 @@ export class CompoundCommand implements Command {
const alreadyRedone: Command[] = [];

try {
this.commands.forEach(command => {
for (const command of this.commands) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not also await the redo in the line below to make sure that the order is properly kept and there are no conflicts?

} catch (err) {
alreadyRedone.forEach(command => command.undo());
for (const command of alreadyRedone) {
command.undo();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not also await the undo here to make sure that the order is properly kept and there are no problems?

@tortmayr tortmayr requested review from martin-fleck-at and removed request for planger February 28, 2023 15:16
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

LGTM!

@tortmayr tortmayr merged commit 44aae0c into main Feb 28, 2023
@tortmayr tortmayr deleted the fix branch February 28, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants