Add new read-only interfaces for all remaining model types #14917
Conversation
|
Judging from the test failures it looks like |
Not sure why this is required, doesn't make much sense.
| /// <summary> | ||
| /// The best-effort metadata representing this set. In the case metadata differs between contained beatmaps, one is arbitrarily chosen. | ||
| /// </summary> | ||
| IBeatmapMetadataInfo? Metadata { get; } |
There was a problem hiding this comment.
Is this nullability a transitional measure? I don't imagine this will be allowed to be null when the whole series of changes concludes, will it?
There was a problem hiding this comment.
I'm still not sure. I brought this up for discussion last week but didn't get a solid answer, so for now have not changed the structure. We need to figure how/where we are going to attach metadata to cause the least issues. For not I avoided change.
osu.Game/Rulesets/IRulesetInfo.cs
Outdated
| // overwrite the pre-populated RulesetInfo with a potentially database attached copy. | ||
| // ruleset.RulesetInfo = this; |
There was a problem hiding this comment.
This one gives me a bit of anxiety - looks like this was a hack to bypass some EF idiosyncrasy, but this being available and potentially callable by mistake feels like a footgun waiting to fire. I'd probably trim this for now unless it's really needed somewhere.
There was a problem hiding this comment.
I'm going to leave it there for now, but I have added a TODO. I'll revisit this when I rebase my RulesetStore implementation. We may want this to be attached if it's a common pattern to access ruleset.RulesetInfo expecting to get a realm-based object, but I'd hope we avoid that pattern in the first place. Although that's what we have to IBeatmap right now, so a bit up in the air until I check usages.
Co-authored-by: Bartłomiej Dach <[email protected]>
|
going to dismiss my review since the blockers look to be resolved. however i am not getting why |
| /// <summary> | ||
| /// All beatmaps contained in this set. | ||
| /// </summary> | ||
| IEnumerable<IBeatmapInfo> Beatmaps { get; } |
There was a problem hiding this comment.
Shouldn't this be named BeatmapInfos to be in line with the new naming?
There was a problem hiding this comment.
Very unsure about that.. just because BeatmapInfos sounds weird.
|
|
||
| // overwrite the pre-populated RulesetInfo with a potentially database attached copy. | ||
| // TODO: figure if we still want/need this after switching to realm. | ||
| // ruleset.RulesetInfo = this; |
There was a problem hiding this comment.
Shouldn't this be uncommented for the time being, since we haven't switched to realm yet?
There was a problem hiding this comment.
This method is unused for now (the one in RulesetInfo is still there). Can remove the interface level method for the time being if that feels better.
| /// <summary> | ||
| /// A user-presentable display title representing this metadata. | ||
| /// </summary> | ||
| string DisplayTitle |
There was a problem hiding this comment.
Not sure about all these default implementations. On the one hand it makes sense from the POV of being able to override these members' implementations, but on the other hand they seem like abuse and maybe extension methods would fit better.
There was a problem hiding this comment.
Will change to extension methods, I think we want to avoid interface defaults everywhere at this point.
|
|
||
| // temporary helper methods until we figure which metadata should be where. | ||
| private static IBeatmapMetadataInfo getClosestMetadata(IBeatmapInfo beatmapInfo) => | ||
| beatmapInfo.Metadata ?? beatmapInfo.BeatmapSet?.Metadata ?? new BeatmapMetadata(); |
There was a problem hiding this comment.
Unsure whether this should be a
| beatmapInfo.Metadata ?? beatmapInfo.BeatmapSet?.Metadata ?? new BeatmapMetadata(); | |
| beatmapInfo.Metadata ?? beatmapInfo.BeatmapSet.Metadata ?? new BeatmapMetadata(); |
as the inspection suggests, or if BeatmapSet should be marked nullable on the interface. There are several other places that nullcheck the property so probably safest to go with the latter for now?
There was a problem hiding this comment.
I think nullable is the safest option for now, yep. Will require revisiting once things are locked in place.
There was a problem hiding this comment.
For what it's worth, I made this change due to an actual null ref, so it can happen.
The provides a baseline for the structure of models going forward. They can/should be used in any cases we only want to allow read-only access to models.