Package handle template #2137
Package handle template #2137
Conversation
fiskus
commented
Mar 29, 2021
•
fiskus
commented
- Unit tests
- Automated tests (e.g. Preflight)
- Documentation
- Changelog entry
Codecov Report
@@ Coverage Diff @@
## master #2137 +/- ##
==========================================
- Coverage 44.20% 44.19% -0.02%
==========================================
Files 488 488
Lines 23640 23678 +38
Branches 3014 3019 +5
==========================================
+ Hits 10450 10464 +14
- Misses 12344 12368 +24
Partials 846 846
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| hasValidationErrors: true, | ||
| form: true, |
There was a problem hiding this comment.
Removed form only, and sort other properties alphabetically
| hasValidationErrors: true, | ||
| form: true, | ||
| initialValues: true, |
There was a problem hiding this comment.
Also, I added initialValues
shared/schemas/workflows.yml.json
Outdated
| } | ||
| } | ||
| } | ||
| }, | ||
| "properties": { | ||
| "version": { | ||
| "const": "1" |
There was a problem hiding this comment.
@sir-sigurd I don't know should I increment version? I've added additional property package_handle, probably version should be 1.1 now?
There was a problem hiding this comment.
i'm not sure if we decided on some global approach to schema versioning, but i'd say we should increment minor version to indicate feature addition while preserving compatibility
| ) | ||
|
|
||
| const [initialValues, setInitialValues] = React.useState({ | ||
| files: initialFiles, |
There was a problem hiding this comment.
this way files initial value wont be updated on input props change (it shouldnt be a problem in practice bc it wont be changing while the dialog is open, but anyways, we should keep that in mind).
btw why did you consolidate all the initial values into one object and put it into state var?
what are the benefits of this vs using separate computed and state vars?
There was a problem hiding this comment.
Just one benefit — it works :)
I couldn't make it work with updating initialValue per field, they just don't update
| directory: undefined, | ||
| username: undefined, | ||
| }), | ||
| ).toBe('/') |
There was a problem hiding this comment.
this expectation doesnt match the description ("should return null")
There was a problem hiding this comment.
seems like this belongs to the it below
shared/schemas/workflows.yml.json
Outdated
| } | ||
| } | ||
| } | ||
| }, | ||
| "properties": { | ||
| "version": { | ||
| "const": "1" |
There was a problem hiding this comment.
i'm not sure if we decided on some global approach to schema versioning, but i'd say we should increment minor version to indicate feature addition while preserving compatibility
| @@ -7,6 +7,25 @@ | |||
| "workflows" | |||
| ], | |||
| "additionalProperties": false, | |||
| "definitions": { | |||
There was a problem hiding this comment.
do we have duplicate schema files in python code and catalog?
|
probably |
|
Was done here #2364 |
