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

Stopgap: MongoDB $jsonSchema #3953

Merged
merged 5 commits into from
May 5, 2023
Merged

Stopgap: MongoDB $jsonSchema #3953

merged 5 commits into from
May 5, 2023

Conversation

Druue
Copy link
Contributor

@Druue Druue commented May 4, 2023

@Druue Druue requested a review from a team as a code owner May 4, 2023 15:30
@Druue Druue changed the title StopGap: MongoDB $jsonSchema Stopgap: MongoDB $jsonSchema May 4, 2023
Comment on lines 151 to 156
if let Some(walker) = doc_count.collection_walker {
if walker.has_schema() {
model.documentation("json schema msg")
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds the documentation message that goes into the schema file above the model

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make sure we agree on the message to render, before accepting json schema msg in this PR :)

Comment on lines 413 to 417
render_warnings(
"The following models have a JSON Schema defined in the database, which is not yet fully supported. Read more: https://pris.ly/d/todo",
&self.json_schema_defined,
f
)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds the cli warning message

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, let's make sure we give the pris.ly link a proper URL. https://pris.ly/d/mongodb-jsonschema should be good enough

@Druue Druue added this to the 4.14.0 milestone May 4, 2023
@codspeed-hq
Copy link

codspeed-hq bot commented May 4, 2023

CodSpeed Performance Report

Merging #3953 gap/intro-jsonschema (89febb3) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 11 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

Copy link
Contributor

@jkomyno jkomyno left a comment

Choose a reason for hiding this comment

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

Good PR so far! I'm requesting changes just so we don't accidentally forget about changing a few strings before merging

Comment on lines 151 to 156
if let Some(walker) = doc_count.collection_walker {
if walker.has_schema() {
model.documentation("json schema msg")
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make sure we agree on the message to render, before accepting json schema msg in this PR :)

Comment on lines 151 to 155
if let Some(walker) = doc_count.collection_walker {
if walker.has_schema() {
model.documentation("json schema msg")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be tempted to refactor this into an Option::filter just to avoid two nested ifs, but this is just a minor nit

Comment on lines 413 to 417
render_warnings(
"The following models have a JSON Schema defined in the database, which is not yet fully supported. Read more: https://pris.ly/d/todo",
&self.json_schema_defined,
f
)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, let's make sure we give the pris.ly link a proper URL. https://pris.ly/d/mongodb-jsonschema should be good enough

Copy link
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

The logic looks good 👍 I think we only need the messages work, indeed.

@jkomyno jkomyno merged commit bd111df into main May 5, 2023
48 checks passed
@jkomyno jkomyno deleted the gap/intro-jsonschema branch May 5, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants