feat(marxan-run): zip input & output directories #316
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). marxan – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan/9p2TuvHZcVAa7RR1YTkSvD3RTMqd marxan-storybook – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/JBnG1YsJcYvVwVuXRNAvtKijzmwV |
|
|
||
| get( | ||
| type: 'INPUTDIR' | 'OUTPUTDIR', | ||
| within: Workspace, |
There was a problem hiding this comment.
I think Workspace is not entirely required here, maybe consider passing just the workingDirectory?
There was a problem hiding this comment.
It could but the intention was to provide this via Workspace to ensure it is what it is supposed to be instead raw string.
Not strongly convinced to either option tho. Do you see any strong benefits by passing only path?
@Dyostiq
There was a problem hiding this comment.
From my point of view, it violates the principle of least knowledge a bit and makes the code harder to test as you need to pass the entire workspace instead of only the part. Then, the big object propagates through the stack just only to pass the path. If you think that passing a raw string is not as expressive as you'd like it to be, maybe wrapping it in an object is a solution – but still, you can pass only really needed data.
| await this.metadataRepository.save(scenarioId, workspace, { | ||
| stdOutput, | ||
| stdErr, | ||
| }); |
There was a problem hiding this comment.
according to my previous comment, the workspace path could be passed here instead of the entire workspace
| @Column({ name: `std_out`, nullable: true, type: 'character varying' }) | ||
| stdOutput?: string | null; | ||
|
|
||
| @Column({ name: `std_err`, nullable: true, type: 'character varying' }) | ||
| stdError?: string | null; |
There was a problem hiding this comment.
maybe we could keep the same names in columns, so std_ouput and std_error
| const config = this.reader.read( | ||
| joinPath(within.workingDirectory, 'input.dat'), | ||
| ); | ||
| const matcher = new RegExp(`(${type} )(?<dir>.*)$`, 'gm'); |
There was a problem hiding this comment.
shouldn't the regex start with ^?
There was a problem hiding this comment.
@Dyostiq it could but doesn't matter there (multiline & known format)
There was a problem hiding this comment.
matters, as the file may contain comments, so:
a comment like this suggesting using INPUTDIR something
matches the regex
There was a problem hiding this comment.
Gotcha, you are right!
kgajowy
force-pushed
the
feat/MARXAN-498-persist-metadata-after-run-3
branch
from
716cf02
to
9e4b34b
Compare
Substitute this line for a meaningful title for your changes
Overview
Please write a description. If the PR is hard to understand, provide a quick explanation of the code.
Designs
Link to the related design prototypes (if applicable)
Testing instructions
Please explain how to test the PR: ID of a dataset, steps to reach the feature, etc.
Feature relevant tickets
Link to the related task manager tickets
Checklist before submitting
develop.deploying to staging/production, please add brief testing instructions
to the deploy checklist (
docs/deployment-checklist.md)