-
Notifications
You must be signed in to change notification settings - Fork 0
Plug and Play Interface and Adaptations #14
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
base: main
Are you sure you want to change the base?
Conversation
Applies similar edits to SequenceTemplates & SequenceTemplateEditor as were applied to Sequences and SequenceEditor. These factor (some) language-specifics out to a separate package, loaded through the parcel. Notably, we decided to demand a "template language adaptation", rather than attempt to automatically produce a template adaptation from the regular language adaptation. This gives end users the ability to apply different functionality to templates and regular sequences, since templating may interfere with some sequence language adaptation features. This commit does not remove the "language" field from sequence templates, since that will require back end changes. It does remove virtually all the functionality tied to that field, though.
Fully removes the last traces of ISequenceAdaptation, along with a handful of related utility functions.
Add the function to detect library sequences to the language adaptation. This removes the last hard-coded switch between VML and SeqN.
…ature/adaptation-interface
…aptation interface for library sequences
Replace any usage of a library sequence map with just a pure array of sequences. This cleans up the UI side and makes for one less distinct interface for the map. Separate the spec for languages into a base interface and extensions for input and output formats. Set up the actual 'adaptation' to contain a getter for the languages given injected dependencies (just utilities for building Svelte tooptlip components for now). What was previously passed around as the 'adaptation' is now 'PhoenixLanguages', which is I think more apt. Also, fix the tooltips that broke when we extracated from aerie-ui.
Laying "new adaptation interface" to rest in favor of better structured interfaces
Moving all the adaptation stuff to the existing folder of languages. Everything lives together, in harmony, like mother nature intended
Discussion topic: naming?
|
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 so far. I did skip over a lot of UI code that was moved over here as it was already reviewed multiple times.
src/languages/seq-n/seq-n.ts
Outdated
export const SeqnParser = parser; | ||
export const seqnParser = parser; | ||
|
||
// TODO re-name this to something other than seqnLanguage to not conflict with naming convention for the Phoenix language specs |
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.
Let me know what you come up with as I should mimic that on the sasf side as I will have a similar file in sasf
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.
Leaning towards <language>LRLanguage
as this uses the LRLanguage
type from Codemirror directly, so it's a little less ambiguous.
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.
If you make that change can you go to src/languages/satf/grammar/satf-sasf.ts
and update the SatfLanguage
to SatfLRLanguage
as well. It will save me the trouble of doing it and should only affect 3 spots in the code.
import { CommandDictionary, FswCommand, FswCommandArgument, HwCommand } from '@nasa-jpl/aerie-ampcs'; | ||
import { getAllEnumSymbols } from './sequence-utils'; | ||
|
||
export function buildAmpcsCommandTooltip(command: FswCommand | HwCommand): string[] { |
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.
Instead of returning a string[] should we return a type
type CommandTooltip = {
command : string,
description: string
}
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'm partial to string[]
just because it maps directly into the input of the createTooltip
utility, but if there's value added by splitting it out then I'm open to it. Would you want to access those fields for other reasons?
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 don't think I should access those fields directly. My thought is that the function is always return two string values. Instead of returning an array which implies 0 -> n
values could be stored in this array, let's just return those two values as a single type.
/** | ||
* TODO migrate everything here to aerie-time-utils | ||
*/ | ||
import { getDoyTimeComponents } from "@nasa-jpl/aerie-time-utils"; | ||
|
||
/** | ||
* Get a day-of-year timestamp from a given JavaScript Date object. | ||
* @example getDoyTime(new Date(1577779200000)) -> 2019-365T08:00:00 | ||
* @note inverse of getUnixEpochTime | ||
* @note milliseconds will be dropped if all 0s | ||
*/ | ||
export function getDoyTime(date: Date, includeMsecs = true): string { | ||
const { doy, hours, mins, msecs, secs, year } = getDoyTimeComponents(date); | ||
let doyTimestamp = `${year}-${doy}T${hours}:${mins}:${secs}`; | ||
|
||
if (includeMsecs && date.getMilliseconds() > 0) { | ||
doyTimestamp += `.${msecs}`; | ||
} | ||
|
||
return doyTimestamp; | ||
} |
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 remove this file completely and user isoFromJSDate
from the aerie-time-utils
in its place.
export function pluralize(count: number): string { | ||
return count === 1 ? '' : 's'; | ||
} |
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.
Should we move this to the utils/string.ts
import { expect, test } from 'vitest'; | ||
import { pluralize } from './text'; | ||
|
||
test('pluralize', () => { | ||
expect(pluralize(0)).toBe('s'); | ||
expect(pluralize(1)).toBe(''); | ||
expect(pluralize(10)).toBe('s'); | ||
}); |
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.
should we move this to the utils/string.test.ts
- Renamed all the SeqN files to be called "seq-n" instead of "sequence" - Restructured how the SeqN adaptation is built to make it easier to modify, both for Clipper's adaptation and for SeqN handlebars - Moved some utilities out of SeqN space that are used more broadly - Removed hard-coded list of globals in favor of passing them into SeqN utils that need them
Work to go: