-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add a new set of interfaces to hide runtime members during model building #24017
Conversation
e8d9e62
to
2c72d43
Compare
/// <summary> | ||
/// Indicates whether the function is read-only. | ||
/// </summary> | ||
public override bool IsReadOnly => ((Annotatable)Model).IsReadOnly; |
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.
We now have two concepts of read-only-ness:
- In the interface hierarchy (read-only = does not (necessarily) support runtime annotations)
- In the model's state (read-only = model has been finalized, no longer mutatable)
If I'm understanding everything right, they're also opposites: finalization also roughly corresponds to transitioning from IReadOnlyModel to IModel; so the model becomes read-only in one sense (can no longer mutate it because it's finalized), and is kinda no longer read-only in the other (we can now put runtime annotations on it).
I guess if we found another name it would be slightly better, e.g. IBuildTimeModel instead of IReadOnlyModel.
My personal preference would be to simply call it IModelBase, IIndexBase, which also avoids the confusion of "why does mutable inherit from read-only". We would then need to rename ITypeBase and IPropertyBase to, say, IRuntimeTypeBase and IRuntimeTypeBase - that would technically be a breaking change, though only in intermediate interfaces so unlikely to have real impact.
Also OK with leaving as-is :)
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.
Personally I like IReadOnlyModel
because it doesn't expose any mutation API, independently of the state of the implementing objects. This follows the IList
/IReadOnlyList
convention.
The model state IsReadOnly
is really an internal concept, with the new interfaces callers won't run into readonly/mutable exceptions unless they cast to a different interface, so they don't need to be concerned about it.
test/EFCore.Relational.Specification.Tests/TestUtilities/RelationalDatabaseCleaner.cs
Outdated
Show resolved
Hide resolved
a606008
to
c654378
Compare
Hello @AndriySvyryd! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
c654378
to
1e09225
Compare
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at [email protected] to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
1e09225
to
1e8f1f6
Compare
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at [email protected] to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
@@ -16,7 +16,7 @@ namespace Microsoft.EntityFrameworkCore.Metadata | |||
/// Once the model is built, <see cref="IKey" /> represents a read-only view of the same metadata. | |||
/// </para> | |||
/// </summary> | |||
public interface IMutableKey : IMutableAnnotatable, IKey | |||
public interface IMutableKey : IMutableAnnotatable, IReadOnlyKey |
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.
couldn't resist seeing that all other classes put IReadOnly* before IMutableAnnotatable.
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.
Will fix in a later PR
Part of #19213
The PRs will get smaller after this one