-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add Load and Activate Real Time Commands to SeqN #1543
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test cases and functionality are good.
My comment on the seq-json types is because distinct literal values help with control flow analysis. It also feels like we have a lot of type assertions.
type: stepNode.name === 'Load' ? 'load' : 'activate', | ||
}; | ||
} as Activate | Load | ImmediateActivate | ImmediateLoad; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a different type
attribute on Activate and ImmediateActivate in seq-json-schema? Same thing for Load and ImmediateLoad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still a little confused about you comment Did you want me to split this up like below
if (stepNode.name === 'Activate') {
return {
args,
description,
engine,
epoch,
metadata,
models,
sequence,
...(!isRTC ? { time } : {}),
type: 'activate',
};
}
return {
args,
description,
engine,
epoch,
metadata,
models,
sequence,
...(!isRTC ? { time } : {}),
type: 'load',
};
Or did you want to go to the schema and change ImmediateActivate's type to type: "immediate_activate"
Similar with Load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current schema change was what @cartermak, Mike Schaffer, and I agreed on so we will have to go back to them and see if they are okay with changing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also split this up so it's clear what logic branch generated what type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow reply -- I don't see an issue with amending the type
attribute on the immediate activate and load steps to say immediate_load
and immediate_activate
, although it is redundant with those objects being in the immediate_commands
array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent, I think you will need to change the ImmediateFswCommand
object type as well to type: 'ImmediateFswCommand';
export interface ImmediateFswCommand {
args: Args;
description?: Description;
metadata?: Metadata;
/**
* Command stem
*/
stem: string;
type?: 'command'; <----- Change to ImmediateFswCommand
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joswig new schema changes in place now. I am also using overloaded
methods
type: stepNode.name === 'Load' ? 'load' : 'activate', | ||
}; | ||
} as Activate | Load | ImmediateActivate | ImmediateLoad; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also split this up so it's clear what logic branch generated what type?
32613ac
to
684ea67
Compare
684ea67
to
a7ab412
Compare
a7ab412
to
4e383b3
Compare
4e383b3
to
c882421
Compare
* Allow for optional timetag so they can be used as immediate commands
* Support the new immediateActivate and immediateLoad objects in the SeqJson * Update the test
c882421
to
4603932
Compare
Summary
Added
immediate_activate
andimmediate_load
to SeqN.SeqJson