-
Notifications
You must be signed in to change notification settings - Fork 2
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
Introduce MigrationType #897
Conversation
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.
This is a great idea and makes much more readable code. 👍
I had a trivial suggestion for one piece of code, which you can ignore if you think it doesn't help.
forceEstimated = MigrationType(cohortSpec) match { | ||
case Membership2023Monthlies => true | ||
case Membership2023Annuals => true | ||
case _ => false | ||
} |
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 this is an improvement or not:
forceEstimated = MigrationType(cohortSpec) match { | |
case Membership2023Monthlies => true | |
case Membership2023Annuals => true | |
case _ => false | |
} | |
forceEstimated = MigrationType(cohortSpec) == Membership2023Monthlies || MigrationType(cohortSpec) == Membership2023Annuals |
val forceEstimated = MigrationType(cohortSpec) match { | ||
case Membership2023Monthlies => true | ||
case Membership2023Annuals => true | ||
case _ => false | ||
} |
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.
This is same as above. Could possibly be extracted
@kelvin-chappell
I am going to leave it as it is, because I am just about to add a new migration type, and will refactor (make those simplifications) once the new one is in :) |
When we introduced the 2023 membership price migrations, we introduced the concept of parametrisation of basic functions with
CohortSpec
s. Back then we got away withif (cohort detections) then ... else (else if ...)
the existing code, but with more of such customised migrations coming, it was time for a refactoring.For this we introduce
sealed trait MigrationType
, leading to a much cleaner and more readable code.MigrationType
does not identity a migration (Sometime several migrations map to a unique migration type, for instance both Membership 2023 Batch 1 and Batch 2 both map to typeMembership2023Monthlies
, whereas Membership 2023 Batch 3 identifies withMembership2023Annuals
. It simply helps identify common code used by possibly more than one migration.All pre 2023 migrations map to type
Legacy
.