sarbian / ModuleManager

177 stars 95 forks source link

Deleting the last key by negative index does not work; deleting keys by value deletes all of them #183

Open JonnyOThan opened 7 months ago

JonnyOThan commented 7 months ago

Possibly related to #75?

Given a node like:

THING
{
  transform = airlock
  transform = airlockTopB
}

and a patch targeting that node,

-transform,-1 = delete does nothing -transform[airlockTopB] = delete deletes all of them -transform,1 = delete seems to work, but of course it's super fragile

Lisias commented 6 months ago

Negative indexes are not implemented. Please read the Forum about (or at very least, the documentation).

-transform[airlockTopB] is a silent syntax error.

What you want to accomplish may be possible using MM_PATCH_LOOP, something like

@THING:HAS[#transform[airlockTopB]]
{
    <something to rule out values different of airlockTopB, and delete it when found>
    MM_PATCH_LOOP { }
}

MM patches are not a programming language. They are a convoluted FSM declaring notation.

JonnyOThan commented 6 months ago

Negative indexes are not implemented. Please read the Forum about (or at very least, the documentation).

Yes, the documentation says:

Delete - ! or - Delete a node or value...For nodes, all options are available to select the node, including indexes. ...For values, again all options available.

(,<index-or-*>)? : Index to select particular match zero-based. 0 is the first node, -1 is the last node.

And obviously deleting ALL keys when trying to select by value is a bug.

Lisias commented 6 months ago

And obviously deleting ALL keys when trying to select by value is a bug.

Yes and no.

There's nothing in the documentation saying that -transform[airlockTopB] = delete should delete just one with the value airlockTopB. This syntax just doesn't exists.

What I think it's happening is the code finds the first [ and automatically thinks it's parsing an ARRAY, doesn't understand a thing, and the code degenerates into "everything you find".

Check https://github.com/sarbian/ModuleManager/wiki/Module-Manager-Handbook#arrays

The bug is not -transform[airlockTopB] = delete deleting all instances, the bug is MM not raising an error on an evident syntax error and just ignoring the problem, degenerating the declaration to -transform,*[0] = delete implicitly.

Semantic matters.

JonnyOThan commented 6 months ago

Ah sure I did learn on re-reading the documentation that deleting a key always deletes all of them unlike nodes. So it seems like it's silently ignoring the value selector. Although again, the documentation says "all options available" but that appears to not actually be correct.

Lisias commented 6 months ago

The documentation sucks, no doubt.

Lisias commented 6 months ago

Since I opened my big mouth here, I need to clarify: there're code to handle negative indexes - but for some nodes only, and ~never~ for some values.