Namespace unbind

[ghost]

Summary of changes

Potentially API-breaking for external plugin stores. If accepted, schedule for inclusion in 7.0

Draft PR rebasing and extending PR #1094 to bring up to finalising accept/reject level (then it can be closed one way or the other)

Adds unbind(prefix: str) -> None method to Graph API, propagated downwards from Graph to individual store implementations (SimpleMemory, Memory, BerkeleyDB, etc).

Checklist

• Checked that there aren't other open pull requests for the same change (rebases PR Fix for Issue #543 (Namespace unbind required) including trie and other modification  #1094).
• Added tests for any changes that have a runtime impact.
• Checked that all tests and type checking passes.
• For changes that have a potential impact on users of this project:
  • Updated relevant documentation to avoid inaccuracies. TBD if Draft approved
  • Considered adding additional documentation. TBD if Draft approved
  • Considered adding an example in ./examples for new features. TBD if Draft approved
  • Considered updating our changelog (CHANGELOG.md). TBD if Draft approved
• Considered granting push permissions to the PR branch,
  so maintainers can fix minor issues and keep your PR up to date.

[aucampia]

As prefix should be supplied as a str I think it is best to omit this custom handling, if anything it can maybe be a TypeError, but I think avoiding custom handling for prefix is None is best. If someone does it they can live with the consequences if it does not fail.

Suggested change

	if prefix is None:
	return

[ghost]

True, coherent type-hinting would resolve this more neatly and I was lazy.

My excuse is - that from a meta-perspective I'm not sure that this PR is desirable in that adding an explicit unbind is arguably not worth the effort when the functionality already exists via bind(<prefix>, None) From this perspective, it's more of a documentation / docstring issue.

[aucampia]

I'm not sure that this PR is desirable in that adding an explicit unbind is arguably not worth the effort when the functionality already exists via bind(<prefix>, None) From this perspective, it's more of a documentation / docstring issue.

If this is the case we can possibly implement this by checking if the underlying store implements unbind, and if not call bind(, None) - but I'm not sure if all stores will work with this either.

[ghost]

If this is the case we can possibly implement this by checking if the underlying store implements unbind, and if not call bind(, None) - but I'm not sure if all stores will work with this either.

Oh, looks like I was mistaken in believing that there was existing functionality for unbinding via binding to None

def test_extant_graph_unbind(tmp_path: Path, store_name: str) -> None:
    graph = make_graph(tmp_path, store_name)
    graph.bind("", EGNSSUB_V0)
    check_ns(graph, {"": EGNSSUB_V0})

    graph.bind("", None)

    assert list(graph.namespaces()) == [
        ("", URIRef("http://example.com/sub/v0")),
        ("default1", URIRef("None")),
    ]


def test_extant_store_unbind(tmp_path: Path, store_name: str) -> None:
    graph = make_graph(tmp_path, store_name)
    graph.bind("", EGNSSUB_V0)
    check_ns(graph, {"": EGNSSUB_V0})

    graph.store.bind("", None)

    if store_name == "BerkeleyDB":
        assert list(graph.store.namespaces()) == [
            ("", URIRef("http://example.com/sub/v0"))
        ]
    else:
        assert list(graph.store.namespaces()) == [("", None)]

I guess that appreciably changes matters: the rebased PR is worth submitting, so coherent type-hinting it is then 😄

[ghost]

Okay, I've added more type hinting (perhaps clumsily, it's not one of my strong points). Had to protect berkeleydb from trying to apply decode("utf-8) to None although the tests do show that mypy will report the type error.

[ghost]

Okay, I've added more type hinting (perhaps clumsily, it's not one of my strong points).

Definitely not, sigh. Changed param namespace type to str throughout, reflecting contemporary usage, seems to satisfy everything except the Type[DOAP] in test/util/earl.py, so I've temporarily added type ignore to those. I guess URIRef and Namespace satisfy some behavioural criterion that allows them to be str whereas Type doesn't.

[coveralls]

Coverage increased (+0.02%) to 88.534% when pulling 456408f on gjhiggins:namespace-unbind into 7d8cab6 on RDFLib:master.

[aucampia]

@gjhiggins the change looks good, I will think of ways which we can incorporate it into master and keep it there until we release version 7.

I'm thinking we add rdflib._internal.version_7 = False and then add this in if rdflib._internal.version_7:. We can then use always_true and always_false from mypy to run type checking, and we can wrap these tests in if rdflib._internal.version_7: also.

That way we can run CI on them, and once we release version 7 we just change rdflib._internal.version_7 = True.

Would like to know what others think of this approach. CC: @mwatts15