add removal of nodes / branches

[nwesem]

This PR tries to solve issue https://github.com/COVESA/vss-tools/issues/194, by adding a delete attribute. So in the base specification or from overlays, we can now set delete: true on a branch or node which will delete them from the created tree.

Please refer to ./docs/vspec2x_arch.md to see an example. I added tests for all exporters but still would appreciate it if you gave this a quick spin to check if all necessary functionality has been implemented.

Thanks! Looking forward to your comments

[erikbosch]

There is a conflict, I would suggest that you do a squash and rebase. But apart from that it looks good.

[nwesem]

There is a conflict, I would suggest that you do a squash and rebase. But apart from that it looks good.

Hi @erikbosch, I'm having trouble to finish the rebase because of the obsolete and tests directories, is there a small chance that in this merge pre-commit wasn't run or that the upgrade of pre-commit is causing it? Same thing occurs when I run pre-commit hooks in the current master branch..

[erikbosch]

The Android part merged isn't conformant with pre-commit requirements (it is old code we do not want to touch). I can do a try and check how it works for me.

[erikbosch]

As there are merge-commits I think a rebase might trigger pre-commit checks, and they do not pass for the Android-related part. This could possibly be solved by first squashing. I tried this (upstream is official COVESA repo)

git fetch upstream pull/342/head:pr342
git fetch upstream
git checkout pr342
# Squash all by interactive rebase
git rebase -i 469bfa346b27e8
# Then rebase on latest master
# A few easily fixed conflicts
git rebase upstream/master
# After fixing them
git rebase --continue

I uploaded the result for reference and created a dummy-PR in my own fork to verify that build works

https://github.com/erikbosch/vss-tools/pull/4

[nwesem]

thx @erikbosch, I tried it and it seems to be working fine. I found a bug though, currently deleting from overlay doesn't work as expected, will get back asap.

[nwesem]

Is there anyone that could help me solve this bug?

I added two more test "overlay" files that are supposed to remove a test node or branch from a base test.vspec but even though the overlay is loaded correctly and the delete element is detected as True, the delete element doesn't get overwritten by the overlay and therefore the node or branch is not deleted. Am I using it wrong? Do you have any ideas? I temporarily pushed the code here https://github.com/nwesem/vss-tools/pull/5

[erikbosch]

@nwesem - I took a look and I think I found the problem

The problem seems to be that you delete the node from the overlay before merge, not from the final tree after merge. If you modify your function like this so that it actually does not remove the node you will see that the removal check will be called 3 times. Once before the merge and twice during the merge (I think)

    def check_for_removal(self):
        """Caution uses base node as input."""
        for node in LevelOrderIter(self):
            if node.delete:
                logging.info(f"Node {node.qualified_name()} will now be deleted - HAHA NOT!.")
                # node.parent = None
                # node.children = []

I think you should run the deletion after the merge. I did a test adding it to the function below and together with disabling check_for_removal it seems to give the right result, and the check is only performed once. You likely do not want to add the deletion to that function, at least not without renaming it, so some refactoring might be needed. If you experience other "strange merge errors" consider applying #346 to make sure it is not related to that error.

def verify_mandatory_attributes(node, abort_on_unknown_attribute: bool):
    """
    Verify that mandatory attributes are present.
    Need to be checked first after overlays (if any) have been applied, as attributes are not
    mandatory in individual files but only in the final tree
    """
    if isinstance(node, VSSNode):
        if node.delete:
            logging.info(f"Node {node.qualified_name()} will now be deleted - BUT NOW!.")
            node.parent = None
            node.children = []
        node.verify_attributes(abort_on_unknown_attribute)
        for child in node.children:
            verify_mandatory_attributes(child, abort_on_unknown_attribute)

[nwesem]

thx a lot @erikbosch, I have to get used to the codebase a bit more to solve these things, obviously it was deleted before the merge :grinning:

[nwesem]

created a squashed and rebased version of this PR here https://github.com/COVESA/vss-tools/pull/348

therefore closing this PR