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.

Sign up for GitHub

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

Jump to bottom

Add Valid::assume_valid_ref #768

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Add Valid::assume_valid_ref #768

merged 3 commits into from
Dec 4, 2023

Conversation

SimonSapin
Copy link
Contributor

No description provided.

@SimonSapin SimonSapin self-assigned this Dec 1, 2023
@SimonSapin
Copy link
Contributor Author

federation-next needs this to upgrade to beta 8+

goto-bus-stop
goto-bus-stop approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@goto-bus-stop goto-bus-stop left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense. though would it make sense for fed-next to clone the document anyways, if it has to do modifications? Using schema.whatever.make_mut() through a reference would mutate the input right?

@SimonSapin
Copy link
Contributor Author

The FederationSchema struct owns a Schema and some methods mutate it. This PR enables converting to &Valid<Schema> (for use in FieldSet::parse) from &Valid<FederationSchema> which also has the non-mutating methods.

Before this PR, ValidFederationSchema would have to be an entirely separate type that owns Valid<Schema> and duplicates non-mutating methods.

@goto-bus-stop
Copy link
Member

Ahhh right I get it, thanks!

@SimonSapin SimonSapin enabled auto-merge (squash) December 4, 2023 09:14
@SimonSapin SimonSapin merged commit 2c07460 into main Dec 4, 2023
@SimonSapin SimonSapin deleted the valid-ref branch December 4, 2023 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goto-bus-stop goto-bus-stop goto-bus-stop approved these changes

@lrlna lrlna Awaiting requested review from lrlna lrlna is a code owner

Assignees

@SimonSapin SimonSapin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SimonSapin @goto-bus-stop