-
Notifications
You must be signed in to change notification settings - Fork 94
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
Conversation
@@ -18,4 +18,4 @@ jobs: | |||
- name: Lint | |||
uses: golangci/golangci-lint-action@v2 | |||
with: | |||
version: v1.42 | |||
version: v1.45 |
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.
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"` |
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.
Pants was so much better... 🤣
"maxLength": 50, | ||
"minLength": 1, | ||
"pattern": "[0-9]{1,4}", | ||
"type": "string" | ||
"pattern": "[0-9]{1,4}" |
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'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.
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.
That's correct, but this is just a test for the examples, the content isn't that meaningful :-)
Insted of completely removing the support for 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 |
@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. |
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.... |
@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 In all my projects now we avoid the use of the |
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? |
I hadn't tried the branch directly, but I did it just now, and I can confirm it seems to work as expected. |
Set of changes that fixes:
This PR replaces #21