osuAkatsuki / bancho.py

An osu! server for the generic public, optimized for maintainability in modern python
https://akatsuki.gg
MIT License
213 stars 129 forks source link

fix: `Beatmap.url` returning an incomplete URL #545

Closed minisbett closed 10 months ago

minisbett commented 11 months ago

Describe your changes

Beatmap.url returns a URL in the format /beatmapsets/{self.set.id}/{self.id}. However, the mode is missing here (e.g. /beatmapsets/1/75 vs /beatmapsets/1#osu/75). Since we don't have an in-built way to get the string of the mode (mode[:2] wouldn't work since catch is fruits here), the easiest way to solve this is by simply using the /b/{self.id} format.

Related Issues / Projects

Checklist

tsunyoku commented 11 months ago

/beatmap does not exist on osu.ppy.sh - it should be /b

but i know these redirects will be gone soon (they're old shit from old.ppy.sh), so i don't think this is particularly a good idea

minisbett commented 11 months ago

/beatmap does not exist on osu.ppy.sh - it should be /b

but i know these redirects will be gone soon (they're old shit from old.ppy.sh), so i don't think this is particularly a good idea

/b is going to be removed? man But I think short-term this is a good way to solve it in bpy, it's just a simple click on merge and when the time has come to work on this to act on the official servers' changes

tsunyoku commented 11 months ago

/beatmap does not exist on osu.ppy.sh - it should be /b

but i know these redirects will be gone soon (they're old shit from old.ppy.sh), so i don't think this is particularly a good idea

/b is going to be removed? man

But I think short-term this is a good way to solve it in bpy, it's just a simple click on merge and when the time has come to work on this to act on the official servers' changes

or, you could add one function to map the mode to the correct string and use the current url format...

g1-1-1 commented 11 months ago

/beatmap does not exist on osu.ppy.sh - it should be /b

but i know these redirects will be gone soon (they're old shit from old.ppy.sh), so i don't think this is particularly a good idea

i don't think the /b/ redirect won't be removed any time soon because it will just contribute to massive linkrot and i know peppy would care about something like that -- not saying your suggestions are invalid or anything but it would be weird for peppy to remove something like that with how old his game is

minisbett commented 11 months ago

/beatmap does not exist on osu.ppy.sh - it should be /b but i know these redirects will be gone soon (they're old shit from old.ppy.sh), so i don't think this is particularly a good idea

i don't think the /b/ redirect won't be removed any time soon because it will just contribute to massive linkrot and i know peppy would care about something like that -- not saying your suggestions are invalid or anything but it would be weird for peppy to remove something like that with how old his game is

same + I don't see why he'd remove it, yes it might be from old.ppy.sh but it's still a good feature because you can have links without knowing a set id

tsunyoku commented 11 months ago

this is becoming off-topic, this pr isn't a debate on if peppy will keep the old endpoint. i'd much prefer you just added a map for mode->string and used the new url format.

i say this also because the ingame direct popup on click requires the new url format, and if we move to /b then these won't work as the embeds we send use this property.

minisbett commented 11 months ago

this is becoming off-topic, this pr isn't a debate on if peppy will keep the old endpoint. i'd much prefer you just added a map for mode->string and used the new url format.

i say this also because the ingame direct popup on click requires the new url format, and if we move to /b then these won't work as the embeds we send use this property.

That's a good point. Would I just add a new property? Like mode.as_url or smth?

tsunyoku commented 11 months ago

this is becoming off-topic, this pr isn't a debate on if peppy will keep the old endpoint. i'd much prefer you just added a map for mode->string and used the new url format.

i say this also because the ingame direct popup on click requires the new url format, and if we move to /b then these won't work as the embeds we send use this property.

That's a good point. Would I just add a new property? Like mode.as_url or smth?

something like Mode.osu_string is good i think (use a property), i don't want to couple the mode string to a url specifically - we may use it in the future for osu api stuff

cmyui commented 10 months ago

Btw https://osu.ppy.sh/beatmapsets/141#/315 also works - without the mode

cmyui commented 10 months ago

Updated a couple of other cases to make the PR consistent.

This is fine for now to fix the immediate issue - feel free to make a follow-up to update to use the new urls.