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

docs(contrib): Start guidelines for schema design #15037

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

epage
Copy link
Contributor

@epage epage commented Jan 8, 2025

What does this PR try to resolve?

This was inspired by a recent Cargo team discussion on whether we should generally elide default values.
This will also help with https://rust-lang.github.io/rust-project-goals/2025h1/cargo-plumbing.html

Case studies in schema design:

How should we test and review this PR?

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2025

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 8, 2025
@@ -0,0 +1,29 @@
# Data Schemas
Copy link
Member

Choose a reason for hiding this comment

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

Should we have an overview of what this "data schemas" refer to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some context

@epage epage force-pushed the schema-design branch 2 times, most recently from 4f50d9b to e323d10 Compare January 8, 2025 22:50
src/doc/contrib/src/implementation/schemas.md Outdated Show resolved Hide resolved
- For output, prefer [jsonlines](https://jsonlines.org/) as it allows streaming output and flexibility to mix content (e.g. adding diagnostics to output that didn't prevously have it
- `deny_unknown_fields` should not be used to allow evolution of formats, including feature gating

## Schema Evolution Strategies
Copy link
Member

Choose a reason for hiding this comment

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

When reading this, each bullet point is a bit disconnected to me.
I can't tell what it means exactly by evolution strategies. Maybe it is a pretty common term but I didn't know?

Also like "When reading data, add new fields", I had a hard time interpreting it as part of evolution strategies. For "Versioned with the edition" I am able to guess it means "When a schema needs to change some existing field, versioning it with edition", though the whole paragraph still looks pretty sparse and needs some guesses for me to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to expand on it a bit more. Hope it helps!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. That is good enough for me as a maintainer. Not sure about casual contributors, but we can always iterate later :)

This was inspired by a recent Cargo team discussion on whether we should
generally elide default values.
This will also help with https://rust-lang.github.io/rust-project-goals/2025h1/cargo-plumbing.html

Case studies in schema design:
- rust-lang#14506
- rust-lang#10543
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

I don't think this need any FCP as it is just some design principles we've used throughout our meeting. It just has never been written down.

Now we have a place for. Perhaps in the future we expand it to an automation to check our code.

- For output, prefer [jsonlines](https://jsonlines.org/) as it allows streaming output and flexibility to mix content (e.g. adding diagnostics to output that didn't prevously have it
- `deny_unknown_fields` should not be used to allow evolution of formats, including feature gating

## Schema Evolution Strategies
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. That is good enough for me as a maintainer. Not sure about casual contributors, but we can always iterate later :)

@weihanglo weihanglo added this pull request to the merge queue Jan 9, 2025
Merged via the queue into rust-lang:master with commit fdaccd4 Jan 9, 2025
21 checks passed
@epage epage deleted the schema-design branch January 9, 2025 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants