-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add module: Base64 #40
Conversation
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.
👍 Great work, just a few comments to address, and question about possibly splitting base64 it up into different modules, or making the param non-optional
src/string/Base64.ts
Outdated
* @category Model | ||
*/ | ||
export type SchemableParams1<S extends URIS> = ( | ||
options?: Base64SchemableOptions |
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'm open to optional params, but I ended up choosing against it for UUID because otherwise it doesn't make much sense why there's a function call for the combinator:
const User = SC.make(S =>
S.struct({
token: S.Base64()
})
)
vs an explicit params, which make it clearer why it's a function call:
const User = SC.make(S =>
S.struct({
token: S.Base64({ urlSafe: true })
})
)
And like you mentioned, an alternative for these simple configs would be to split them up into different modules
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 ended up adding an additional module. I'll open a new issue to cover this. And it's probably worth discussing this some more since I expect we'll need to find out a middle ground between representing all possible variants, and having an API that is reasonably compact and appropriate for most use cases.
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.
Created #44
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.
Beautiful 🎉
…o dm/add-string--base64
closes #23
closes #44
Note:
I diverted slightly from the(Ended up adding a new module,validator.js
module by making these url-safe by default. I don't have a great rationale for doing this, aside from it felt like the more correct general use-case.Base64Url
.yarn generate:schemables
Follow-up [edited]
validator.js
.