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

Refactor metadata lookup flows #23822

Merged
merged 10 commits into from Feb 11, 2024
Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 8, 2023

RFC. Probably wouldn't recommend merging into any hotfix releases if anything of the sort is coming soon.

This precedes a fix to a bug in the metadata lookup flow I found a while back, loosely described in footnote 1 here. There are two reasons why I went off on this detour:

  • Currently BeatmapUpdaterMetadataLookup has the logic of applying the metadata of the map duplicated in two places (depending on whether API or the local metadata snapshot is being used), and I would have had to fix up both places as both are susceptible to the same bug that I want to fix. I find that kinda weird as the two places are doing precisely the same thing, based on the same type of data, which is just sourced from a different place. So this restructure kind of logically made sense to me.
  • After working with this code a while, it turns out that there are a lot of undocumented and not-obvious expectations from the metadata lookup flow which are currently not exercised by tests in any way. I wanted to get this code under a test harness before I started working it further, to make sure I don't accidentally break anything along the way.

All of the above is mostly argumentative and subjective, and none of it is strictly necessary for fixing the bug that I want to fix, but I kind of like the end result. So I want to see what the reaction to this will be.

@peppy
Copy link
Sponsor Member

peppy commented Feb 11, 2024

This took a bit to get through. But coming with a fresh look on the structure of these classes, I think I can agree that I like the end result.

One thing I'll mention is that I think there might need to be an end goal of using the local cache more frequently, or adjusting the throughput of api requests as this api fetch process is the main blocker of import speed. That is obviously something that can come as a follow-up later on.

@bdach I hope you still have the aforementioned bug fix branch sitting around to follow up with 😅

@peppy peppy enabled auto-merge February 11, 2024 12:10
@peppy peppy merged commit 33d4e8f into ppy:master Feb 11, 2024
10 of 11 checks passed
@bdach bdach deleted the refactor-metadata-lookup-sources branch February 12, 2024 09:16
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.

None yet

2 participants