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

Use base ctor to remove unnecessary property copies #269

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

smoogipoo
Copy link
Contributor

See discussion in #265 (comment). Turns out I added these copies to the base class in the osu-side PR (ppy/osu#31637).

{
RoomID = room.RoomID;
Host = room.Host?.User;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is technically different than what the base ctor that's being called in the new code does. Did you test that this doesn't cause any weird issues? I'd like to think that it doesn't but this is the sort of nightmare model juggling stuff that breaks more often than I'd want to see.

Copy link
Contributor Author

@smoogipoo smoogipoo Mar 20, 2025

Choose a reason for hiding this comment

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

Yeah, this is fine.

First, this value is overwritten by osu!web.

Second, .User should actually always be null in spectator because it's only used osu-side as a post-processing variable, so the base ctor is more correct in any case, though it doesn't matter because of the above. Now that I say it out loud, the .User property should be removed from this model altogether, but that's for another time.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

seems to check out, just gonna merge and hope that we notice breakage (if any) on staging

@bdach bdach merged commit c1f3367 into ppy:master Mar 20, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants