-
Notifications
You must be signed in to change notification settings - Fork 75
oopsy: add translation for oopsy NetRegexes #947
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| this.triggers.push({ | ||
| ...looseTrigger, | ||
| localRegex: Regexes.parse(Array.isArray(regex) ? Regexes.anyOf(regex) : regex), |
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 code, checking if regex is an Array, was written a long time ago when we didn't have the NetRegex translation system.
cf. git blame of damage_tracker.ts. This was written 5 years ago.
example from old days
netRegex: NetRegexes.startsUsing({ source: 'The Manipulator', id: '13E7', capture: false }),
netRegexDe: NetRegexes.startsUsing({ source: 'Manipulator', id: '13E7', capture: false }),
netRegexFr: NetRegexes.startsUsing({ source: 'Manipulateur', id: '13E7', capture: false }),
netRegexJa: NetRegexes.startsUsing({ source: 'マニピュレーター', id: '13E7', capture: false }),
netRegexCn: NetRegexes.startsUsing({ source: '操纵者', id: '13E7', capture: false }),
netRegexKo: NetRegexes.startsUsing({ source: '조종자', id: '13E7', capture: false }),So, I'm removing the if condition, and just use regex (localRegex from my commits). - see 4ed345d
Important
Please leave comments if this change breaks cactbot.
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.
Localized netRegexes were still being used in oopsy. I've replaced them with timelineReplace. - see
601287e
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, unfortunately, a breaking change as currently written because technically users could still have user js files using this syntax:
{
id: 'Some Custom Trigger',
netRegex: [/^21\|[^|]+\|10001234/, /^22\|[^|]+\|10001234/],
}Yet another reason to do some major cleanup before next expansion, with regards to #907.
I don't really have time right now to sit down and think through the logic required to allow this without breaking potential user triggers, but maybe something like this?
const netRegexAny = Array.isArray(regex) ? Regexes.anyOf(regex) : regex;
const translated = translateRegex(netRegexAny, parserLang, timelineReplace);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.
|
Question regarding translations here: |
I'm not sure what you're asking about, can you ask me more specific or give me an example to check if it works? I'm not understanding well about how cactbot is handling RSV. ... Edit: Ah, well... I edited this whole comment. I have to read more code in |
|
RE naming, we could use something like "AbilityNameReplacement" or "TranslationReplacements" or something along those lines |
1. Trigger Initialization:
|
|
Just a heads-up. If you want to rename fields, such breaking changes might not be merged until the release of version 8.0. |
I'd say is to keep the same for now (but maybe move it to types.d) and make an issue to suggest the breaking change rename as prep for 8.0 |
6688197 to
b60196c
Compare
b60196c to
59cf3b9
Compare
|
@Souma-Sumire @jacob-keller Oops, sorry 😓 I've reset commits and removed the renamings. I'll add a comment on #907 |
|
|
||
| this.triggers.push({ | ||
| ...looseTrigger, | ||
| localRegex: Regexes.parse(Array.isArray(regex) ? Regexes.anyOf(regex) : regex), |
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, unfortunately, a breaking change as currently written because technically users could still have user js files using this syntax:
{
id: 'Some Custom Trigger',
netRegex: [/^21\|[^|]+\|10001234/, /^22\|[^|]+\|10001234/],
}Yet another reason to do some major cleanup before next expansion, with regards to #907.
I don't really have time right now to sit down and think through the logic required to allow this without breaking potential user triggers, but maybe something like this?
const netRegexAny = Array.isArray(regex) ? Regexes.anyOf(regex) : regex;
const translated = translateRegex(netRegexAny, parserLang, timelineReplace);This reverts commit 4ed345d.


Fixes #764
timelineReplacefieldexample
ui\oopsyraidsy\data\06-ew\ultimate\the_omega_protocol.tsImportant
Refactoring Plan:
TimelineReplacementI used
TimelineReplacementfrom raidboss, but I believe this type should now be defined in a common place, intypes\trigger.d.tsI think.I'm also concerned about the name of the type. Oopsy does not have the timeline. I'm still for keeping the original name,
TimelineReplacementthough. It'll be quite hard to find a good name to cover both raidboss and oopsy.I am planning to refactor this in this PR. Feel free to make any comments about this plan.