Skip to content
This repository has been archived by the owner on Jul 16, 2024. It is now read-only.

feat: added toString for Schema classes #605

Merged
merged 7 commits into from
Dec 11, 2023

Conversation

jessekelly881
Copy link
Contributor

@jessekelly881 jessekelly881 commented Nov 25, 2023

Adds a default toString method to schema classes that uses the compiled Pretty instance.

Copy link

changeset-bot bot commented Nov 25, 2023

🦋 Changeset detected

Latest commit: 46f823d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@effect/schema Patch

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

@tim-smart
Copy link
Member

This should be done upstream in the effect/Data module :)

@jessekelly881
Copy link
Contributor Author

jessekelly881 commented Dec 3, 2023

@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. "some(1)" vs "{ tag: "Some", /* ... */ }"). :P

@jessekelly881
Copy link
Contributor Author

@tim-smart :(

@@ -4441,6 +4445,10 @@ const makeClass = <I, A>(

static [TypeId] = variance

toString() {
return `${this.constructor.name}(${pretty(this as any)})`
Copy link
Member

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?

Copy link
Member

@tim-smart tim-smart Dec 11, 2023

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))
         }),

Copy link
Contributor Author

@jessekelly881 jessekelly881 Dec 11, 2023

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.

@tim-smart
Copy link
Member

@gcanti did you have any additional thoughts?

@gcanti
Copy link
Contributor

gcanti commented Dec 11, 2023

LGTM

@tim-smart tim-smart merged commit c728880 into Effect-TS:main Dec 11, 2023
1 check passed
@github-actions github-actions bot mentioned this pull request Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants