-
-
Notifications
You must be signed in to change notification settings - Fork 37
feat: added toString for Schema classes #605
Conversation
🦋 Changeset detectedLatest commit: 46f823d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This should be done upstream in the effect/Data module :) |
@tim-smart It's done here to take advantage of the @effect/schema/Pretty module. If we add it to the Data module directly the generated strings won't be as nice(e.g. |
829e6ee
to
27de7e5
Compare
@tim-smart :( |
27de7e5
to
e41e063
Compare
@@ -4441,6 +4445,10 @@ const makeClass = <I, A>( | |||
|
|||
static [TypeId] = variance | |||
|
|||
toString() { | |||
return `${this.constructor.name}(${pretty(this as any)})` |
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.
Can this be added to the ast as a pretty hook?
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 was thinking this:
diff --git a/src/Schema.ts b/src/Schema.ts
index 3a76808..1c461c5 100644
--- a/src/Schema.ts
+++ b/src/Schema.ts
@@ -4433,7 +4433,6 @@ const makeClass = <I, A>(
additionalProps?: any
): any => {
const validator = Parser.validateSync(selfSchema)
- const pretty = Pretty.to(selfSchema)
return class extends Base {
constructor(props: any = {}, disableValidation = false) {
@@ -4446,7 +4445,7 @@ const makeClass = <I, A>(
static [TypeId] = variance
toString() {
- return `${this.constructor.name}(${pretty(this as any)})`
+ return Pretty.to(this.constructor as any)(this)
}
static pipe() {
@@ -4462,7 +4461,8 @@ const makeClass = <I, A>(
ParseResult.succeed(input)
: ParseResult.fail(ParseResult.type(ast, input)), {
[AST.DescriptionAnnotationId]: `an instance of ${this.name}`,
- [hooks.PrettyHookId]: () => (props: any) => new this(props).toString(),
+ [hooks.PrettyHookId]: (struct: any) => (self: any) =>
+ `${self.constructor.name}(${struct(self)})`,
[hooks.ArbitraryHookId]: (struct: any) => (fc: any) =>
struct(fc).map((props: any) => new this(props))
}),
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.
Hmm. I don't know. Currently the Duration.toString and BigDecimal.toString both include the name of the type. It probably makes since to include it here too. Also, it makes sense to compile the pretty fn outside of toString for efficiency and potentially safety(it could throw).
https://github.com/Effect-TS/effect/blob/2c64bf0f003c9542b0b6efd571c516337b32b6af/src/BigDecimal.ts#L66
The change to the pretty hook makes sense though. I updated that.
@gcanti did you have any additional thoughts? |
LGTM |
Adds a default toString method to schema classes that uses the compiled Pretty instance.