Use more stringent checks when applying online metadata #27143
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tadata After ppy#23362 I'm not sure the `LocallyModified` check is doing anything useful. It was confusing me really hard when trying to parse this logic, and it can misbehave (as `None` will also pass the check).
bdach
added
type:online
area:import
smoogipoo
approved these changes
Feb 13, 2024
smoogipoo
approved these changes
Feb 13, 2024
peppy
reviewed
Feb 13, 2024
There was a problem hiding this comment.
I was still at the tail end of reviewing the assertions here, but will just add that it looks okay to my eye at first glance. It's hard to test coverage of this without testing against real-world import scenarios, so getting this into users' hands is paramount.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Long overdue promised follow-up to #23822.
Please examine the added test cases for the baseline expectations I want to enforce, and please check commit messages for rationale behind each change. Skepticism is very much welcome and warranted, especially around 87702b3. I'm not sure if the semantics I set down make much sense to anyone but me.