-
-
Notifications
You must be signed in to change notification settings - Fork 350
initial contribution, invited by Juan #1880
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @agilevic! Thanks a lot for your contribution! I noticed that the following required information is missing or incomplete: issue reference Please update the PR description to include this information. You can find placeholders in the PR template for these items. Thanks a lot! |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1880 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 30 30
Lines 633 633
Branches 196 196
=========================================
Hits 633 633 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@json-schema-org/docs-team @json-schema-org/tsc Can you help with your own reviews for this blog post? @agilevic and I have been talking for a while and SlashDB is a very nice "database gateway" that generates JSON Schema. I think it will be a great addition for the case studies! |
|
@jviotti do you want to unblock this? There is no issue reference to give. Thanks. |
|
No worries. Feel free to ignore that check. But let me ping the team again for reviews |
jdesrosiers
left a comment
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 fine to me. My only concern is that I'm not a fan of the "type": [whatever, "null"] pattern and I'm concerned that people are going to read this article and think that's a recommended way to describe optional properties. I think it would be helpful to explain why you need to support two different ways for a property to not be present (defined with null or not defined) and note that this is not typical and not always the right approach.
As a style suggestion, I found the schemas hard to read because they span multiple screens that I need to scroll through. I suggest using,
"type": ["string", "null"]instead of,
"type": [
"string",
"null"
]This will make the schemas significantly more compact and I think easier to read.
gregsdennis
left a comment
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 an interesting article. It's neat that you've been able to integrate JSON Schema this tightly into the DB.
I do have some comments, but they're largely editorial. The base content looks great. Thanks for writing this up.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Hi @agilevic! Thanks a lot for your contribution! I noticed that the following required information is missing or incomplete: issue reference Please update the PR description to include this information. You can find placeholders in the PR template for these items. Thanks a lot! |
|
Hi @agilevic! Thanks a lot for your contribution! I noticed that the following required information is missing or incomplete: issue reference Please update the PR description to include this information. You can find placeholders in the PR template for these items. Thanks a lot! |
|
Thank you all for reviews. I have only now gotten around to make the edits. Thank you for your patience. |
What kind of change does this PR introduce?
New blog post; invited to contribute by @jviotti
You can change the type to Engineering if more appropriate.