-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add command for applying simple mod multiplier changes #246
Conversation
+ "FROM `scores` " | ||
+ "WHERE `id` BETWEEN @lastId AND (@lastId + @batchSize - 1) " | ||
+ "AND `ruleset_id` = @rulesetId " | ||
+ "AND JSON_SEARCH(`data`, 'one', @modAcronym, NULL, '$.mods[*].acronym') IS NOT NULL", |
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.
Wasn't sure whether to do this sql-side or c#-side. I think this should be somewhat painless because EXPLAIN
pegs this as a where filter over a range PK lookup so it shouldn't be that slow.
|| OldMultiplier == null | ||
|| NewMultiplier == null) | ||
{ | ||
await Console.Error.WriteLineAsync("One or more required parameters is missing."); |
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.
Kinda weird to have optional arguments which are required (usually I'd use standard arguments in place of this, but I guess your goal is to have things named?)
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.
Yeah I didn't want to have them purely positional to prevent screw-ups from swapping them or something.
{ | ||
[Command(Name = "score", Description = "Runs batch processing on score totals for scores and users.")] | ||
[Subcommand(typeof(ChangeModMultiplierCommand))] | ||
public class ScoreCommands |
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 would have probably just put the command in maintenance, given there's already similar score repair commands in there?
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.
At the time of writing I wasn't sure if there were going to be any others, for e.g. user total recalculations. But I think it's pretty clear to me that it's not required anymore, will move.
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.
Add `Serializable` attribute to suppress CI complaint
5ec4745
to
dac88a3
Compare
As per discord, not sure why this is close/delete. We will still need this command and the structure likely won't change by much at all. Unless the thought is that we'd run a batch sql (ie. via ASS |
Opening as draft because while this works, this is the absolute simplest tool for the job at hand (namely, changing classic mod multiplier). Its limitations are:
If the properties above are desired now, then my proposal is to split this command into two.
data
json blob, because it doesn't require costly schema changes to execute.