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

Fix OpenUserProfile links having multiple argument types #24022

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

frenzibyte
Copy link
Member

OpenUserProfile having more than one argument type makes it hard to retrieve the URL from LinkDetails as done in the PR linked above, and generally convolutes the process of handling OpenUserProfile basically anywhere.

Besides that, LinkDetails having arguments of type object is already bad on its own, but the whole structure is bad anyway.

@peppy
Copy link
Member

peppy commented Jun 24, 2023

I'm not sure this is achieving much. I guess you're trying to box everything into "can be shoved in URL", but that's not the case. Usernames need to be prefixed with an @ going forward (see ppy/osu-web#10303).

Probably want an extension method (or just property) on IUser to generate the URL, and then make everything use IUser instead?

@frenzibyte
Copy link
Member Author

The idea of this PR is more of trying to use one simple type for LinkDetails arguments (right now on OpenUserProfile there are shockingly two types and multiple ways to handle the link appropriately, 1. handle IUser with only username, 2. handle IUser with online ID and invalid username, 3. handle string).

With the PRs you linked above, I'll imagine us just updating the LinkDetails construction of opening user profiles to append a @ when dealing with a username.

@frenzibyte
Copy link
Member Author

I guess I wouldn't mind changing the argument type to IUser as long as the handling is still in place.

@peppy peppy self-requested a review June 26, 2023 04:12
@peppy peppy merged commit 55ab27c into ppy:master Jun 26, 2023
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.

3 participants