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

Add validate_t #136

Closed
wants to merge 1 commit into from
Closed

Conversation

metanivek
Copy link

@metanivek metanivek commented Mar 4, 2022

While experimenting with atdgen, I came across an edge case when trying validations and untypable JSON.

If I use the following, I get a build warning about deprecation of the json type.

type json <ocaml module="Yojson.Safe"> = abstract

If I add an explicit type (t="t") to address the warning, compilation fails with Unbound value Yojson.Safe.validate_t, so this PR adds it. Since it appears that the json type is disappearing, I also made validate_json an alias to validate_t as the canonical definition.

For compatibility with atdgen validation and untypable JSON.

e.g. `type json <ocaml module="Yojson.Safe" t="t"> = abstract`
@Leonidas-from-XIV
Copy link
Member

The type json will be removed removed in 2.0.0, atdgen will need to update to use Yojson.Safe.t.

In fact, I feel like we should probably also remove validate_json since this function doesn't seem to serve any purpose, @panglesd?

@metanivek
Copy link
Author

Got it. I’m happy to remove validate_json here. atdgen does work with Yojson.Safe.t but it needs validate_t to exist when using validations.

@Leonidas-from-XIV
Copy link
Member

I think in such case it would be more reasonable to define a module that wraps Yojson.Safe.t in your code and provides validate_t. Or change atdgen to allow injecting a validation function in another way.

@metanivek
Copy link
Author

Agreed. This seems like a good opportunity to break this connection between the two libraries since it is really an atdgen concern.

@Leonidas-from-XIV
Copy link
Member

Closing this in favor of #136. Thanks @metanivek for pointing our attention to the fact that we have old left-over code that doesn't work, let's make 2.0.0 the best release we can.

@metanivek metanivek deleted the add_validate_t branch March 7, 2022 16:29
@metanivek
Copy link
Author

@Leonidas-from-XIV happy to help!

@MisterDA
Copy link

Just had the same problem. Could you explain how to turn off the validation for the json type? Every combination I'm using turn out to be wrong with the code atdgen.2.2.1 generates.
You could also point to this issue from the changelog.

@metanivek
Copy link
Author

@MisterDA I missed your comment previously, but I came back to this the other day and realized it wasn't working. I made a PR that should fix this as of atdgen.2.9.1 release. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants