Skip to content
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

add removal of nodes / branches v2 #348

Merged
merged 5 commits into from
Apr 12, 2024
Merged

add removal of nodes / branches v2 #348

merged 5 commits into from
Apr 12, 2024

Conversation

nwesem
Copy link
Contributor

@nwesem nwesem commented Apr 9, 2024

This is a squashed and rebased version of PR #342, please follow the link for more details

Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
@nwesem
Copy link
Contributor Author

nwesem commented Apr 9, 2024

hi @erikbosch @ppb2020 created this squashed and rebased PR, it includes all changes that we spoke about in PR #342

Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
@erikbosch
Copy link
Collaborator

MoM:

  • Please review until EOB Thursday
  • If no remarks OK to merge Friday

Copy link
Collaborator

@ppb2020 ppb2020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few additional comments. The PR can go in as is with my comments being addressed in the future, if this is simpler for you.

docs/vspec2x_arch.md Outdated Show resolved Hide resolved
docs/vspec2x_arch.md Outdated Show resolved Hide resolved
delete: true
```

Also, the delement element can be used on instances after they have been expanded. If you want to delete a node or
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's neat that this works, BTW. Cool!

As a side question, what kind of error is generated if one tries to delete a non-existing instance? Is it differentiated from an error message generated by trying to delete a node that has no instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done even added a test for this case, it fails at this position in the code:

...
        # Type presence should have been tested earlier, but is tested here again for completeness    
        if "type" not in self.source_dict.keys():                                                     
            logging.error("Invalid VSS element %s, must have type", self.name)                        
            sys.exit(-1)

docs/vspec2x_arch.md Show resolved Hide resolved
Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
@nwesem
Copy link
Contributor Author

nwesem commented Apr 10, 2024

Should be ready for merge now

Copy link
Collaborator

@erikbosch erikbosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, waiting until Friday to see if anyone else has any comments

@erikbosch erikbosch merged commit 5044632 into COVESA:master Apr 12, 2024
5 checks passed
erikbosch pushed a commit that referenced this pull request May 3, 2024
* add 'delete' element for node/branch removal

Signed-off-by: Niclas Wesemann <niclas.wesemann@motius.de>
@nwesem nwesem deleted the pr342 branch July 3, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants