-
Notifications
You must be signed in to change notification settings - Fork 4
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
[CSP-2182] add schema generator to connector utils #29
[CSP-2182] add schema generator to connector utils #29
Conversation
…rry/CSP-2182/add-schema-generator-to-connector-utils
README.md
Outdated
| keys | <code>Object</code> | The keys that you wish to extract from the schema with any override values. | | ||
| operation | <code>String</code> | The name of the connector operation that you are generating the schema for. | | ||
|
||
For more information on how to use the schema generator, please see the [notion page](https://www.notion.so/trayio/Schema-Generation-42f5e57341d1473c9a61b9b0e57b31db). |
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.
wondering if its a good idea to be making the repo docs dependent on notion links?
we could put the rest of the docs in a separate .md file in the repo instead of in notion and link it, kinda like how the raw http docs are separate in falafel repo
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.
Yeah, I was uncomfortable doing that.
I'll link to a new md file as you suggest.
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.
Could you maybe add some tests using the 'test' connector that exists in the repo? It would be a good show of usage for future maintainers.
lib/generateInputSchema.js
Outdated
// eslint-disable-next-line no-console | ||
console.warn(MISSING_KEYS_MESSAGE); | ||
} | ||
// eslint-disable-next-line no-console |
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.
Might as well ignore this in the entire file
|
||
/** | ||
* Deep recursive iteration through a full schema object definition. | ||
* Returns a flat array of objects specifying the missing keys. |
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.
Do we need JSDOC if we aren't actually exposing this publicly?
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 did wonder that myself. Removed.
lib/index.js
Outdated
@@ -38,24 +39,29 @@ module.exports = { | |||
}, | |||
deepMap: { | |||
deepMapKeys: (collection, iteratee) => { | |||
// eslint-disable-next-line no-console |
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.
Might as well ignore this in entire file
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.
As discussed in slack basing the tests of a schema.js file and standalone tests rather than through the test runner.
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.
Nice work man!
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.
Nice :)
…rry/CSP-2182/add-schema-generator-to-connector-utils
Added schema generation helper to utils.
Jira ticket: CSP-2182.
There should be enough info in the README and the notion page to see what this is all about.