-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feature/block relative and notes seqn #1600
base: develop
Are you sure you want to change the base?
Conversation
cb332f5
to
f3455d7
Compare
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.
Good chance you've already discussed this, but in case you haven't - should a negative duration be allowed for BLOCK_RELATIVE
? I would assume that we don't want to be able to time steps prior to the block start, but I could be missing some use case.
This is a great question for @shaheerk94, we use the same definition as Epoch currently which includes negatives. |
Actually, I just tested it and SEQGEN does allow negative FROM_REQUEST_START durations. In SASF:
In SSF (compiled products):
So we should allow negative BLOCK_RELATIVE durations. |
Quality Gate failedFailed conditions |
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.
Looks good. I have a question about the Time1
type as this seems weird to have this type interface defined.
} else if (timeTagRelativeNode || timeTagBlockRelativeNode) { | ||
let relativeText = ''; | ||
let from = -1; | ||
let to = -1; | ||
|
||
if (timeTagRelativeNode) { | ||
from = timeTagRelativeNode.from; | ||
to = timeTagRelativeNode.to; | ||
relativeText = text.slice(from + 1, to).trim(); | ||
} else if (timeTagBlockRelativeNode) { | ||
from = timeTagBlockRelativeNode.from; | ||
to = timeTagBlockRelativeNode.to; | ||
relativeText = text.slice(from + 1, to).trim(); | ||
} | ||
|
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.
You can make this an else as it is the last thing in the conditional.
else {
const relativeNode = timeTagRelativeNode || timeTagBlockRelativeNode;
if (!relativeNode) {
return diagnostics;
}
const from = relativeNode.from;
const to = relativeNode.to;
const relativeText = text.slice(from + 1, to).trim();
TimeRelative { 'R'(timeSecond | timeDOY | timeHhmmss) whiteSpace} | ||
|
||
TimeBlockRelative { 'B'timeHhmmss whiteSpace } | ||
TimeBlockRelative { 'B'$[+\-]?(timeSecond | timeDOY | timeHhmmss) whiteSpace } |
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.
I've noticed that we're using timeSecond, timeDOY, and timeHhmmss
quite frequently. We can create a new variable
timeSegments { timeSecond | timeDOY | timeHhmmss} <--- new
TimeAbsolute { 'A'@digit@digit@digit@digit"-"@digit@digit@digit"T"timeHhmmss whiteSpace }
TimeRelative { 'R'timeSegments whiteSpace} <-- update
TimeBlockRelative { 'B'$[+\-]?timeSegments whiteSpace } <-- update
TimeEpoch { 'E'$[+\-]?timeSegments whiteSpace} <-- update
TimeGroundEpoch { 'G'$[+\-]?timeSegments whiteSpace} <-- update
@@ -18,6 +18,7 @@ import type { | |||
StringArgument, | |||
SymbolArgument, | |||
Time, | |||
Time1, |
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.
This is really strange. Why is there a new Time1
type? This new RelativeBlock
Time tag should be in the Time
Type right?
@cartermak @shaheerk94 any thoughts on 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.
It's in the types.ts
...I bet it has something to do with the conditional that we added to enforce that everything except COMMAND_COMPLETE
needs a tag
specified? But reading through types.ts
, it doesn't really make sense. I wonder if the auto-generation is struggling with that conditional logic, or maybe I just am bad at TS.
/**
* Time object
*/
export type Time = Time1 & {
/**
* Relative or absolute time. Required for ABSOLUTE, BLOCK_RELATIVE, COMMAND_RELATIVE, and EPOCH_RELATIVE time types but not COMMAND_COMPLETE.
*/
tag?: string;
/**
* Allowed time types: ABSOLUTE, BLOCK_RELATIVE, COMMAND_RELATIVE, EPOCH_RELATIVE, or COMMAND_COMPLETE.
*/
type: 'ABSOLUTE' | 'BLOCK_RELATIVE' | 'COMMAND_RELATIVE' | 'EPOCH_RELATIVE' | 'COMMAND_COMPLETE';
};
export type Time1 = {
[k: string]: unknown;
};
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.
Yeah I was curious about this as well, but I just included it in the new logic. We could remove from the underlying library and I can update my code.
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.
Have you checked whether it behaves correctly? It seems like all the typing uses Time1
, which doesn't seem to actually enforce time restrictions at all? Or maybe there's some logic I missed.
Adds block relative and notes to our SeqN, hooked up to the latest version of our seqjson library as well.
Tagging Carter and Shaheer for visibility.
@shaheerk94 @cartermak