-
Notifications
You must be signed in to change notification settings - Fork 213
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
v10 migration patch #2021
v10 migration patch #2021
Conversation
[frontend] [Fri Apr 26 02:52:57 UTC 2024] - Deployed 5957d67 to https://genshin-optimizer-prs.github.io/pr/2021/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Fri Apr 26 03:15:37 UTC 2024] - Deployed 466cb69 to https://genshin-optimizer-prs.github.io/pr/2021/frontend (Takes 3-5 minutes after this completes to be available) [Fri Apr 26 05:57:53 UTC 2024] - Deleted deployment |
@@ -172,6 +172,8 @@ export function migrateGOOD(good: IGOOD & IGO): IGOOD & IGO { | |||
const optConfig = | |||
buildSettings.find(({ id }: { id: string }) => id === characterKey) ?? | |||
{} | |||
// Only migrate characters with either a mtarget or an opttarget | |||
if (!customMultiTarget?.length && !optConfig.optimizationTarget) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we even need the opt target thing. Won't this still end up creating 15 loadouts if a support is in 15 teams and has a opt target?
Or am I reading the logic wrong and this will just create one loadout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merging loadouts is complicated logic that probably can't be safely implemented.
This logic avoids creating teams/loadouts for characters that are in GO, but don't have a opt-target selected.
If someone has like 60 characters, but only set up optimizer for like 20 characters, it will leave the other 40 characters that are not in any teams of the core 20 characters to be "no loadout/team"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd kinda be down to only migrating a single loadout per char, where the loadout is the one with multi-opt. conditionals and selected opt target are easy to recreate. tho i guess build constraints might want to be migrated idk. either way needs more thought for further progress, but good changes
Describe your changes
V10 Migration creates a team+loadout for every character in db.
Amend logic to only create a team+loadout if that character had either mtargets or an opt-target selected.
This should reduce some of the migration noise/cleanup.
Teammate loadout for characters with mtargets/optargets are still created, to preserver teammate conditionals.
Issue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-ci
locally to validate format and lint.