template/python.j2: Allow to remove object from object set

[benjarobin]

For simplicity, rebuild the index from scratch instead of trying to remove the object from the index.

But maybe we should try to be clever, and only update the index without rebuilding it.

[JPEWdev]

Yes, this is fine. Just keep in mind the object might still be in the graph if it's referenced indirectly by another object

Rebuilding the whole index is probably the safest for now; I don't expect this to be a very common operation

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
src/shacl2code
__main__.py
context.py
main.py
model.py
urlcontext.py
src/shacl2code/lang
common.py
cpp.py
golang.py
jinja.py
jsonschema.py
python.py
Project Total

This report was generated by python-coverage-comment-action

[JPEWdev]

On second thought, please add some tests :)

[benjarobin]

Sorry for the response delay. But I don't really like the current implementation. This is not great, indeed, if we remove an object still referenced by another element in the graph, this will break everything. I submitted it way too early...
Since I am creating my own subclass, I have moved the remove() function in my implementation, and in this implementation I can do a custom check: Currently, I only allow removing Relationship objects (which should be safe).
Since this implementation is pretty specific (could you still accept that?), and since the current implementation is not good enough, I'm leaning towards closing this pull request.
What do you think? And yes, I will add tests if we are not going to close this pull request.

[JPEWdev]

Sorry for the response delay. But I don't really like the current implementation. This is not great, indeed, if we remove an object still referenced by another element in the graph, this will break everything. I submitted it way too early... Since I am creating my own subclass, I have moved the remove() function in my implementation, and in this implementation I can do a custom check: Currently, I only allow removing Relationship objects (which should be safe). Since this implementation is pretty specific (could you still accept that?), and since the current implementation is not good enough, I'm leaning towards closing this pull request. What do you think? And yes, I will add tests if we are not going to close this pull request.

I don't think the current implementation is completely useless; even if you wanted to do a more thorough removal, you'd still need to do this step to make sure the object wasn't in the root of the object set; this just doesn't do all of what you want.

I don't really want SPDX specific APIs in here, but if you can come of with a generic way of implementing it (which I suspect isn't really that different from what you'd be doing anyway), I'd be fine with adding an API in here for that; maybe like a remove_all API or something like that? The name remove for the implementation in this PR is fine because it's the opposite semantics of the existing add API; so I'd rather not use that name for the more complete removal API (names are hard :) ).

[benjarobin]

For information I do not plan to work on it in the short term: I am quite busy...
For my use case, I can safely remove a Relationship object, since nobody should have a reference to a Relationship object. So the current code is kind of fine for my use case (I replace the check: I only allow to remove Relationship object). So for my use case, I do not need to implement a remove_all API.
And if we also want a remove API, this function should check that the object is not referenced anywhere (for example in a Relationship). This check should be fully generic, so it implies doing a walk over the whole graph, and recursively checking every property that none of them have a reference to the object to be deleted.
The remove_all API should do the same kind of walk, but if it finds a reference in the graph, it should remove it from where it was referenced.

[JPEWdev]

NP. We can leave this open is case you want to come back around to it.