[IMP] targetDependantCommand: avoid dispatch if target is empty #7619
Conversation
|
|
fdamhaut
force-pushed
the
master-avoid-empty-target-flda
branch
from
b44208d to
79b7180
Compare
hokolomopo
left a comment
hokolomopo
left a comment
There was a problem hiding this comment.
Sounds good 👍
Did you check the allowDispatch of all the target dependent commands ? Maybe some were already rejecting the command if it was empty, in which case it's duplicated code.
| } | ||
| ); | ||
|
|
||
| test.each(Object.entries(TEST_COMMANDS).filter(([cmdName, cmd]) => isTargetDependent(cmd)))( |
There was a problem hiding this comment.
We already have OT_TESTS_TARGET_DEPENDANT_COMMANDS constant. But filtering on the command content sounds like a better idea than a static list, could you maybe change OT_TESTS_TARGET_DEPENDANT_COMMANDS (and similar constants) to do that ?
| if (isTargetDependent(cmd) && cmd.target.length === 0) { | ||
| return CommandResult.EmptyTarget; | ||
| } |
There was a problem hiding this comment.
could we do the same for the rangeDependantCommands ? I don't quite get @rrahir comment in the pad about ADD_CONDITIONAL_FORMAT being valid with an empty list of range, we have an explicit check to reject it if the ranges are empty.
3 participants
Description:
description of this task, what is implemented and why it is implemented that way.
Task: 4863945
review checklist