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

Refactor type handling for maps and types, removing YAML #23

Merged
merged 4 commits into from Jul 5, 2022

Conversation

samlown
Copy link
Contributor

@samlown samlown commented May 9, 2022

Set of changes that fixes:

  • Embedding anonymous properties or structs is now made by default in he same way Go Marshals JSON: Structs are inherited, anything else is added as a property.
  • Support for YAML tags has been removed, as it adds a lot of confusion, especially to the anonymous property handling. (I love YAML, but this lib needs to focus on JSON.)
  • If arrays/slices or maps are provided as the root schema, they will be correctly added to the list of definitions, like a struct.

This PR replaces #21

@samlown samlown requested a review from flimzy May 9, 2022 22:19
@samlown samlown mentioned this pull request May 9, 2022
@@ -18,4 +18,4 @@ jobs:
- name: Lint
uses: golangci/golangci-lint-action@v2
with:
version: v1.42
version: v1.45
Copy link

Choose a reason for hiding this comment

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

1.46 just came out yesterday, I think :)


// Set of plants that the user likes
Plants []*nested.Plant `json:"plants" jsonschema:"title=Pants"`
Plants []*nested.Plant `json:"plants" jsonschema:"title=Plants"`
Copy link

Choose a reason for hiding this comment

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

Pants was so much better... 🤣

"maxLength": 50,
"minLength": 1,
"pattern": "[0-9]{1,4}",
"type": "string"
"pattern": "[0-9]{1,4}"
Copy link

Choose a reason for hiding this comment

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

I'm not making sense of this spec.

Is pattern anchored to the beginning/end of the string?

If so, isn't the maxLength effectively 4?

And if not, isn't this the same as "pattern": "[0-9]"?

That is to say: If the pattern isn't anchored, then in effect, we're just looking for a single digit in the string? And if it is anchored, then we'll never have a lenght beyond 4...

But maybe I'm misunderstanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, but this is just a test for the examples, the content isn't that meaningful :-)

@luisdavim
Copy link

luisdavim commented Jun 11, 2022

Insted of completely removing the support for yaml tags, would it be possible to just handle them as json tags? For example, if the a json tag is not found, look for a yaml tag but don't have any special cases for yaml vs json...

Something like:

jsonTagString, exists := f.Tag.Lookup("json")
if !exists {
	jsonTagString, _ = f.Tag.Lookup("yaml")
}
jsonTags := strings.Split(jsonTagString, ",")

It would be cnsistent with the json marshaling/unmarshaling but it wouldn''t force users to add extra json tags just for generating the schema.

@samlown
Copy link
Contributor Author

samlown commented Jun 11, 2022

@luisdavim thanks for your suggestion, but I'd really like to understand your use-case better. I worry about having something in there "half baked" as the YAML specifications are really quite a bit more complex than JSON.

For instance, why would you want to create JSON schema using Go structs that would not be able to read the output data unless converted to YAML?

May I also suggest having a look at https://github.com/invopop/yaml, another project we adopted. It saves having to write all the tags twice as it first converts the YAML to JSON.

@luisdavim
Copy link

luisdavim commented Jun 11, 2022

Hi, the use case is that we have some yaml configuration files and use json schemas to get intellisense suggestions in IDEs using for example https://github.com/redhat-developer/yaml-language-server

I could look into using the project you suggested and converting all my yaml tags to json....

@samlown
Copy link
Contributor Author

samlown commented Jun 13, 2022

@luisdavim That's a good use-case! But please do look into the invopop/yaml lib and get back if you think it's a non-starter! I don't like the ambiguity of using either json or yaml tags, but I was thinking that we could have an opt-in that allows yaml tags to be used, always with a preference for json if present.

In all my projects now we avoid the use of the yaml tag for configurations and stick to json. To me it seems the most robust solution as adding custom UnmarshalJSON methods is way easier than UnmarshalYAML, or both!

@arvidfm
Copy link
Contributor

arvidfm commented Jul 1, 2022

Hi, sorry to bother, just wondering what the status is of this PR? I'm very much interested in the improved handling of anonymous types and slices! (In fact I even ended up implementing the former myself locally.)

@samlown
Copy link
Contributor Author

samlown commented Jul 4, 2022

Hi, sorry to bother, just wondering what the status is of this PR? I'm very much interested in the improved handling of anonymous types and slices! (In fact I even ended up implementing the former myself locally.)

Thanks for reminding me! I've been using this branch in another project directly and forgotten about the release!

Have you tried using this branch directly for your use-case with the anonymous types? Does it work for you if so?

@arvidfm
Copy link
Contributor

arvidfm commented Jul 4, 2022

I hadn't tried the branch directly, but I did it just now, and I can confirm it seems to work as expected.

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.

None yet

4 participants