-
-
Notifications
You must be signed in to change notification settings - Fork 272
[Docs] Updated Examples Section #62
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
Conversation
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
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.
Nice job. I am providing some small suggestions for better consistency among the different example pages.
We'll need more people reviewing the examples.
Co-authored-by: Benjamin Granados <[email protected]>
Co-authored-by: Benjamin Granados <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Thank you! I've made the suggested changes. |
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.
Looks good to me. Nice job!
I'd love to get at least another review.
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.
These examples look good, and it appears you put some time into them. However, I would expect each example to showcase some different feature set.
For instance, if you're starting from the top, "blog post" adds nested properties, a feature that "address" doesn't have. But then "book" just illustrates the same feature set that "address" did. Does having "book," then, hold any real value?
To me, these all look like basically the same example. (Maybe that's needed for this page; I'm not sure.)
pages/learn/json-schema-examples.md
Outdated
|
||
## Blog post | ||
|
||
A schema representing a blog post, including properties like `title`, `content`, `published date`, `author details`, and `tags`. |
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.
If you're going to use the "speech" form of these properties (e.g. "published date" instead of publishedDate
), I think it should be without formatting. Otherwise, just use the actual property names. Similarly for the other summaries. We should be consistent as well.
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.
How about using the publishedDate form instead? Having a space in a key is not something that I have seen widely used in json in my experience.
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.
Yes, that would fall under
Otherwise, just use the actual property names.
nice job on the examples. I believe you spent significant effort on these and the community is grateful for contributions like this. I agree with Greg on the necessity for similar examples. The other thing I would caution is JSON Schema providing too many schema examples of things. You don't want to imply these are the preferred schema objects for the entities listed. Not to discredit your effort, but the schemas provided are also quite basic. No referencing, or composition of multiple schemas is used. No examples of particular keywords like {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"type": "object",
"propertyNames": {
"pattern": "^[a-z_]+$"
},
"patternProperties": {
"": {
"type": "number"
}
}
}
#valid
{
"any_property_name": 33,
"im_a_new_property": 66
}
#invalid
{
"I_am_not_WORKING": 33,
"BIG_FAIL": 66
}
#invalid
{
"any_property_name": "33",
"im_a_new_property": 66
} I believe a majority of JSON Schema users require additional education in those areas and this is where it should be provided. e.g. In an enterprise design, I can guarantee your schema is never straight forward like Just as an example, we have a "person": {
"description": "The candidate personal data",
"$ref": "../../../../../common/person/personType_v01.json"
} |
@gregsdennis @jeremyfiel Thank you for the reviews! I'll definitely modify the examples as I do see how they can seem repetitive and basic. Will research more on how I can improve them with your suggestions and then update the PR soon! |
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
I apologize for the delay in updating this PR 😅 I have added some more properties to the examples such as Please let me know what you think about this. |
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.
It looks like there are some misunderstandings around $dynamic*
and $anchor
. It'd be good to review how those work. Unfortunately we don't really have good docs for $dynamic*
just yet (that I'm aware of) so you'll likely just need to try to read the spec (we can answer questions, too), but here's $anchor
.
Thank you! I'll go over this by the weekend, and update the PR soon. |
Hey @aku1310, firsly, thanks again for taking the time and making the effort to try and improve our learning resources. |
Hi, I apologise for the delay in this PR, I have not been able to properly work on this for a while 😅 Only the use cases in the other examples is left for now. Would it be alright to continue in this PR only? Or I can split it up if that is preferable. I aim to get this update the PR by this weekend as I have gotten free from other work now as well 😅 |
That is totally fine Akanksha! We are very close to get this done. If you need help or feedback let me know and I'll be happy to help. |
Will surely ask, thank you!! |
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Signed-off-by: Akanksha Kushwaha <[email protected]>
Hi, as per the review, I have removed Please let me know what you think about this. |
pages/learn/json-schema-examples.md
Outdated
|
||
```json | ||
{ | ||
"deviceType": "Smartphone", |
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.
The casing here needs to match what the schema expects. But the example looks a lot better now.
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.
Oh yes, I'll just update that 😅
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.
Overall much better. Left one more comment; just a minor thing.
Signed-off-by: Akanksha Kushwaha <[email protected]>
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.
Excellent job @aku1310!
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.
Thanks for all your work on this. Very much appreciated!
In Other Examples:
In Miscellaneous Examples:
In Modelling a File System:
Please let me know what you think about this and any changes required.