Replace_* and Add_* families of operations share a broad semantics of "mutate value if condition matches", but have some idiosyncrasies, here I am taking specifically about transition actions.
About transition actions:
- These two declarations produce different values for action at compilation time:
IF ~~ EXIT // None()
IF ~~ DO ~~ EXIT // Some("")
- when transition is serialized to disk,
Some("") is essentially converted to None():
|
(match t.action with |
|
| None -> () |
|
| Some(s) when |
|
(try let _ = Str.search_forward letter_regexp s 0 in |
|
false |
|
with Not_found -> true) -> () |
|
| Some(real_action) -> flags := !flags lor 4) ; |
(Actually, that regex does more, but it is reasonable to think about is as Some("") -> None() normalizer)
- All
Replace_* operations completely skip None() values.
- All
Add_* operations always test their conditions, if value is None() then it is treated as empty string:
|
match trans.Dlg.action with |
|
| None -> if passes d_when "" then trans.Dlg.action <- Some(action_text) |
|
| Some(str) -> if passes d_when str then |
Now, there is a symmetry within Replace_* and Add_* operation groups, but when combined with serialization behavior it creates an interesting issue when operation behaves differently depending on whether its target was declared locally or read from disk:
| Declaration |
Semantics |
Add_Trans_Action |
Replace_Trans_Action |
Same behavior |
Just declared IF ~~ DO ~~ EXIT |
Can match with condition IF ~^$~ |
Runs |
Runs |
✅ |
Load old IF ~~ DO ~~ EXIT |
Can match with condition IF ~^$~ |
Runs |
Skips |
❌ |
Just declared IF ~~ DO ~~ EXIT |
Update value |
Runs |
Runs |
✅ |
Load old IF ~~ DO ~~ EXIT |
Update value |
Runs |
Skips |
❌ |
Proposal
Add the same "None() is treated as empty string" behavior for all *_Action operations.
While that would break symmetry among Replace_* operations - how axis, it would introduce symmetry for what axis among *_Action operations.
Counter argument:
This breaks compatibility.
Counter counter arguments:
- I could not find a single use of
REPLACE_* with condition. This does not mean that nobody uses that, but points to Replace semantics being naturally similar to IF condition and also might indicate that people already found out that using UNLESS is unreliable so they opted for different solutions.
- Technically it would improve compatibility between mods, because there would be no risk of making transition action "unreplacable", like in this synthetic example:
IF ~~ DO ~initialAction()~ EXIT
...
REPLACE_TRANS_ACTION foo 0 0 ~initialAction()~ ~~
...
REPLACE_TRANS_ACTION foo 0 0 ~^$~ ~anotherAction()~ // this will not run, because at this point action is None()
Replace_*andAdd_*families of operations share a broad semantics of "mutate value if condition matches", but have some idiosyncrasies, here I am taking specifically about transition actions.About transition actions:
Some("")is essentially converted toNone():weidu/src/dlg.ml
Lines 311 to 317 in fc5a92f
(Actually, that regex does more, but it is reasonable to think about is as
Some("") -> None()normalizer)Replace_*operations completely skipNone()values.Add_*operations always test their conditions, if value isNone()then it is treated as empty string:weidu/src/dc.ml
Lines 844 to 846 in fc5a92f
Now, there is a symmetry within
Replace_*andAdd_*operation groups, but when combined with serialization behavior it creates an interesting issue when operation behaves differently depending on whether its target was declared locally or read from disk:IF ~~ DO ~~ EXITIF ~^$~IF ~~ DO ~~ EXITIF ~^$~IF ~~ DO ~~ EXITIF ~~ DO ~~ EXITProposal
Add the same "None() is treated as empty string" behavior for all
*_Actionoperations.While that would break symmetry among
Replace_*operations -howaxis, it would introduce symmetry forwhataxis among*_Actionoperations.Counter argument:
This breaks compatibility.
Counter counter arguments:
REPLACE_*with condition. This does not mean that nobody uses that, but points toReplacesemantics being naturally similar toIFcondition and also might indicate that people already found out that usingUNLESSis unreliable so they opted for different solutions.