Schema helper functions #1197

andresmgot · 2019-10-01T14:51:30Z

Follow up #1196

Actual implementation of the schema helper function. There are two main functions:

retrieveBasicFormParams: Reads the schema and return the list of basic params.
setValues: Reads the give values and modify a certain value.

An issue to mention is that the yaml package doesn't support Typescript definitions (yet), the current package has some missing definitions and it's not usable so we need to work with plan javascript.

absoludity

I'm not 100%, but parts of this look like a re-implementation of parts of lodash - worth taking a look.

dashboard/src/shared/schema.test.ts

absoludity · 2019-10-02T07:45:12Z

dashboard/src/shared/schema.test.ts

+      expect(retrieveBasicFormParams(t.values, t.schema)).toEqual(t.result);
+    });
+  });
+});


👏 Excellent clear tests!

dashboard/src/shared/schema.test.ts

absoludity · 2019-10-02T08:05:02Z

dashboard/src/shared/schema.ts

+  return basicFormParameters;
+}
+
+// getDocumentElem returns the YAML element given a root element


Unless I'm missing something, rootElem here is just an object right? In which case, we can possibly remove (and not have to maintain) this function completely in favor of https://lodash.com/docs/#get ?

Yeah, unfortunately it's not. It's a YAML.ast.Document from https://eemeli.org/yaml/#documents so we cannot use lodash methods.

Ah - I see. Thanks for the reference. So... at least for this function: lodash.get(rootElem.toJSON(), path, defaults)?

Actually, checking the docs, it looks like YAML provides getIn for this reason? It takes an iterable which is the path segments, as in, if we had a document.contents AST representing { foo: { bar: [8, 9] } }, then we should be able to use rootElem.getIn(['foo', 'bar', 1]) to get the value 9? At least, that's what I understand from the docs... I'd be surprised if we needed to implement getValue and setValue ourselves.

oh, nice catch, I understood that differently (I thought that the *In methods only worked with collections/arrays) but it's true that we can use that. Thanks!

The only caveat I have found is that setIn returns an error if the object doesn't define the parent element:

> d.toString() 'a: b\n' > d.setIn(["b", "c"], "foo") Error: Expected YAML collection at b. Remaining path: c

In any case, that's the same limitation that we have now.

dashboard/src/shared/schema.ts

absoludity

Thanks for the info - more comments in-line :)

absoludity · 2019-10-02T11:30:32Z

dashboard/src/shared/schema.ts

+  return basicFormParameters;
+}
+
+// getDocumentElem returns the YAML element given a root element


Ah - I see. Thanks for the reference. So... at least for this function: lodash.get(rootElem.toJSON(), path, defaults)?

Actually, checking the docs, it looks like YAML provides getIn for this reason? It takes an iterable which is the path segments, as in, if we had a document.contents AST representing { foo: { bar: [8, 9] } }, then we should be able to use rootElem.getIn(['foo', 'bar', 1]) to get the value 9? At least, that's what I understand from the docs... I'd be surprised if we needed to implement getValue and setValue ourselves.

dashboard/src/shared/schema.ts

absoludity · 2019-10-02T11:58:52Z

dashboard/src/shared/schema.ts

+  defaultValues: string,
+  schemaProperties: {},
+  parentPath?: string,
+  params?: IBasicFormParam[],


I found this argument odd - why pass the ref to the array as an arg, rather than just relying on the function return value? You could remove this argument - I think - if you...

absoludity · 2019-10-02T12:00:13Z

dashboard/src/shared/schema.ts

+  schemaProperties: {},
+  parentPath?: string,
+  params?: IBasicFormParam[],
+) {


always create a params array here, and then...

absoludity · 2019-10-02T12:00:34Z

dashboard/src/shared/schema.ts

+        // Use the default value either from the JSON schema or the default values
+        value: getDefaultValue(defaultValues, itemPath) || schemaProperties[propertyKey].default,
+      };
+      params = params ? params.concat(newParam) : [newParam];


only ever params.concat(newParam) here, then...

absoludity · 2019-10-02T12:01:38Z

dashboard/src/shared/schema.ts

+    }
+    // If the property is an object, iterate recursively
+    if (schemaProperties[propertyKey].type === "object") {
+      params = lookForFormParams(


params.concat(lookForFormParams(...)) recursively here (but without passing in the params arg, obviously).

Or why do you need to rely on both a return value and an array reference as a function arg?

took me a while to get my head around that one 😅

You are right, the params argument is not needed.

andresmgot · 2019-10-02T14:18:09Z

Apart from your feedback @absoludity I have added the types for yaml and json-schema so the code is easier to read and properly typed. To avoid the issue that getIn and setIn are not defined in the Typescript package, I had to convert the document (YAML.ast.Document) to any. Let me know what you think.

absoludity

Sweet, that's much clearer, thanks for the updates!

Still one thing I'm unsure about - why we call setValue with a string yaml doc and return a string, rather than just keeping a reference to the doc in the form state so that we'd pass the doc, update it, and return the same ref (or not even need to return it)?

But +1 either way.

absoludity · 2019-10-03T04:20:00Z

dashboard/src/shared/schema.ts

+  if (schema && schema.properties) {
+    return lookForFormParams(defaultValues, schema.properties);
+  }
+  return {};


This function looks like it doesn't do much now - other than verify the schema has properties... why not rename lookForFormParams to retrieveBasicFormParams and have call-sites use that (if they have properties)? Given that lookForPormParams no longer has the array arg (which may have been awkward for call-sites to call with an empty array), it now looks exportable?

yes, we can do that, I wanted to split it for being a bit more readable but probably there is not much difference.

absoludity · 2019-10-03T04:23:05Z

dashboard/src/shared/schema.ts

+  const doc = YAML.parseDocument(values);
+  const splittedPath = path.split(".");
+  return (doc as any).getIn(splittedPath) || defaultValue;
+}


Great - getValue and setValue are much simpler now. I'm still not sure why we pass string values into these, rather than passing the actual document. I expected that the form state would be the document, which gets updated with data by the user... converting that to a string and back on each change seems superfluous, but maybe there is context I'm missing?

My reasoning is that the the values.yaml is received as a string and it's always treated as a normal string in the Form component if it doesn't have support for the basic form.

My idea is to have this abstraction layer so the Form always work with the string values.yaml regardless of the implementation of this library. We are using yaml Documents but we could be using regexps or lodash to modify those values. That way, the Form component doesn't depend on the implementation details here.

Also, if we maintain in the Form state the Document we would need to implement there the transformation string<-->Document.

Andres Martinez Gotor added 7 commits October 1, 2019 16:17

Read schema and add username to the basic form

2395af9

Schema func mock

47f1bcf

BasicDeploymentForm tests

62bd18f

Schema helper functions

ccef8fc

Merge usernameInput

8dcf2b4

Fix DeploymentForm test

cc402a9

Remove unnecesary deps

e0247b8

andresmgot requested a review from absoludity October 1, 2019 15:09

absoludity reviewed Oct 2, 2019

View reviewed changes

Andres Martinez Gotor added 2 commits October 2, 2019 12:41

Merge master

cdb8d00

Prettify changes

0c3fc1c

absoludity reviewed Oct 2, 2019

View reviewed changes

Apply feedback

49822c8

andresmgot changed the base branch from usernameInput to master October 2, 2019 14:12

absoludity approved these changes Oct 3, 2019

View reviewed changes

Andres Martinez Gotor added 2 commits October 3, 2019 11:43

Replace logic within lookForParams

e076f9b

Rename var

cbf75c5

andresmgot merged commit ba6dd99 into master Oct 3, 2019

andresmgot deleted the schemaFunctions branch October 3, 2019 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schema helper functions #1197

Schema helper functions #1197

andresmgot commented Oct 1, 2019

absoludity left a comment

absoludity Oct 2, 2019

absoludity Oct 2, 2019

andresmgot Oct 2, 2019

absoludity Oct 2, 2019

andresmgot Oct 2, 2019

absoludity left a comment

absoludity Oct 2, 2019

absoludity Oct 2, 2019

absoludity Oct 2, 2019

absoludity Oct 2, 2019

absoludity Oct 2, 2019

andresmgot Oct 2, 2019

andresmgot commented Oct 2, 2019

absoludity left a comment

absoludity Oct 3, 2019

andresmgot Oct 3, 2019

absoludity Oct 3, 2019

andresmgot Oct 3, 2019 •

edited

Loading

Schema helper functions #1197

Schema helper functions #1197

Conversation

andresmgot commented Oct 1, 2019

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented Oct 2, 2019

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

andresmgot Oct 3, 2019 •

edited

Loading