Skip to content
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

feat: use verbose schema for encoders #446

Merged
merged 4 commits into from
Sep 15, 2024
Merged

feat: use verbose schema for encoders #446

merged 4 commits into from
Sep 15, 2024

Conversation

vdorokhin
Copy link
Contributor

fixes #441

Copy link
Member

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks backwards compatibility. While this change may suit your needs, it is common for people to use canonical forms of the schema.

@vdorokhin
Copy link
Contributor Author

This change breaks backwards compatibility. While this change may suit your needs, it is common for people to use canonical forms of the schema.

With the current implementation, everyone who used the String() method to retrieve the schema will continue receiving the canonical version of the schema. However, if needed, access to other schema data will also be available. In any case, I’m open to making additional changes. I can add a flag to control this behavior. Would that be a suitable option?

@nrwiersma
Copy link
Member

With the current implementation, everyone who used the String() method to retrieve the schema will continue receiving the canonical version of the schema. However, if needed, access to other schema data will also be available. In any case, I’m open to making additional changes.

While true, you are changing the default behaviour of the application. It is reasonable to assume that the same inputs by enlarge produce the same outputs.

I suggest an option to store the "full" output, perhaps WithFullSchema.

@vdorokhin
Copy link
Contributor Author

I added the -fullschema flag to enable this behavior. In the tests, I used it in combination with -encoders for easier verification

@vdorokhin vdorokhin requested a review from nrwiersma September 10, 2024 09:13
Copy link
Member

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉 Thanks for the contribution.

@nrwiersma nrwiersma merged commit a6c674d into hamba:main Sep 15, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Keep a detailed version of schema in case of generation with encoders flag
2 participants