-
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
Import SATF Library Sequence #1592
base: develop
Are you sure you want to change the base?
Conversation
Is this branch supposed to merge into #1574? It looks like a lot of the same changes. |
4c910c8
to
98330e2
Compare
Yeah it is branched from 1574 and since that was merged in the first 4 fell off. It should now all be new code |
98330e2
to
f45cb60
Compare
@@ -59,6 +61,34 @@ | |||
const workspaceId = getSearchParameterNumber(SearchParameters.WORKSPACE_ID); | |||
goto(`${base}/sequencing/new${workspaceId ? `?${SearchParameters.WORKSPACE_ID}=${workspaceId}` : ''}`); | |||
} | |||
|
|||
async function importLibary(): Promise<void> { |
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.
typo in importLibary
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.
Thanks for catching that
@@ -120,10 +120,10 @@ export function getDefaultVariableArgs(parameters: VariableDeclaration[]): strin | |||
return parameter.allowable_values && parameter.allowable_values.length > 0 | |||
? `"${parameter.allowable_values[0]}"` | |||
: parameter.enum_name | |||
? `${parameter.enum_name}` | |||
? `"${parameter.enum_name}"` |
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 update the test for this case?
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.
Sure added a unit test to test this path
name: seqN.name, | ||
parcel_id: parcel, | ||
seq_json: '', | ||
workspace_id: workspaceId ? workspaceId : -1, |
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 will use -1
if workspaceId is 0
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.
ahhh, I fixed this.
workspace_id: workspaceId !== null ? workspaceId : -1
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.
@duranb the dismissal here was to test sonarqube, consider this unresolved
return; | ||
} | ||
let contents = ''; | ||
try { |
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 this try/catch prevent the logic from proceeding if it can't read the file? The importLibrarySequences
effect might be better equipped to handle this logic and also return the contents. Then it can show a failure modal as well.
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 moved things into the effects.importLibrarySequences so everything is handled there. Thank you for the suggestions
b59789e
to
5047f18
Compare
This modal allows you to select a parcel and import a library sequence file
The linter incorrectly reported errors for values within a valid range when multiple ranges were defined. For example, if the ranges were 5-10 and 11-20, the linter would erroneously flag the value 8 as invalid because it falls outside the 11-20 range, even though it is valid within the 5-10 range.
…invalid variable types. Allow autocomplete to succeed and show the ERROR parameter value. The user will see this error in the editor and manually have to fix it.
There shouldn't be any parenthesis and commas in the SeqN generation
5047f18
to
d329635
Compare
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.
testing sonarqube gate, not inspecting contents
Closes #1591
Library Sequence Modal:
Linting Bugfix:
Autocomplete Bugfix:
SATF/SASF Library
TESTING
banana_library.txt