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

Clean up User class #15466

Merged
merged 18 commits into from
Nov 8, 2021
Merged

Clean up User class #15466

merged 18 commits into from
Nov 8, 2021

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Nov 4, 2021

For now I haven't created a non-API version of this class (apart from the simple realm one). We can iterate on that if/when required.

@frenzibyte frenzibyte self-requested a review November 5, 2021 08:38
frenzibyte
frenzibyte previously approved these changes Nov 5, 2021
frenzibyte
frenzibyte previously approved these changes Nov 5, 2021
@frenzibyte frenzibyte dismissed their stale review November 5, 2021 09:31

conflicts (nice timing)

frenzibyte
frenzibyte previously approved these changes Nov 5, 2021
@bdach
Copy link
Collaborator

bdach commented Nov 6, 2021

So I started reviewing this, but I have a big problem with b9983ad. That commit generates 99% of this diff and contains multiple complications that were not explained anywhere, namely:

  • That APIUser already existed but was unused, and was co-opted for the rename instead.
  • That not only User was renamed, but that other related API classes were also renamed (APIPlayStyle, APIUserScoreAggregate)
  • That commit also includes the introduction of IUser.

All of the above make reviewing this as-is a pretty dreadful experience because I'm basically discovering half of the satellite changes by browsing through a 200-file diff. If it's not too much trouble I'd much prefer that it was split to a separate pull, with split commits for each individual refactoring operation performed.

Normally I wouldn't ask for this, but when the diffstat is 2k lines long...

@peppy
Copy link
Sponsor Member Author

peppy commented Nov 6, 2021

can split out those commits if you need. no real explanation though, just matching other recent changes

@bdach
Copy link
Collaborator

bdach commented Nov 6, 2021

I guess I could try and power through with the changes as they are but splitting that part better would make it a lot easier for me to review this.

@peppy peppy changed the title Clean up User class (and rename to APIUser) Clean up User class Nov 7, 2021
@smoogipoo
Copy link
Contributor

I managed to get a crash with this somewhere in the beatmap overlay, but the error was non-descript:

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at osu.Framework.Graphics.Containers.GridContainer.layoutContent()
   at osu.Framework.Graphics.Containers.GridContainer.Update()
   at osu.Framework.Graphics.Drawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()

And I also couldn't repro it. So lgtm.

@peppy
Copy link
Sponsor Member Author

peppy commented Nov 8, 2021

Worrying, but hopefully not related to this PR alone...

@smoogipoo smoogipoo merged commit da0d972 into ppy:master Nov 8, 2021
@peppy peppy deleted the user-class-cleanup branch November 8, 2021 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants