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

Addition of override argument to rdflib.store.Store.bind breaks compatibility #1880

Closed
aucampia opened this issue May 3, 2022 · 1 comment · Fixed by #2018
Closed

Addition of override argument to rdflib.store.Store.bind breaks compatibility #1880

aucampia opened this issue May 3, 2022 · 1 comment · Fixed by #2018
Assignees
Labels

Comments

@aucampia
Copy link
Member

aucampia commented May 3, 2022

See:

@aucampia aucampia changed the title Addition of override to Store.bind breaks compatibility Addition of override argument to rdflib.store.Store.bind breaks compatibility May 3, 2022
@aucampia
Copy link
Member Author

So the way I see it, the addition of "override" fixed a bug, but to fix the bug we need the interface of bind on stores to accept the argument.

I think the most reasonable solution here is to call store bind via this adaptor method:

def _store_bind(self, prefix: str, namespace: URIRef, override: bool) -> None:
try:
self.store.bind(prefix, namespace, override=override)
except TypeError as error:
logger.warning("caught %s", error, exc_info=True)
self.store.bind(prefix, namespace)

This way, if the underlying store does not support the override argument we just call Store.bind without it, and then the bug will still be present.

I'm still testing this and will try refine it a bit so that it does not catch unrelated errors and I will improve the log message.

CC: @RDFLib/core-reviewers

@aucampia aucampia self-assigned this Jul 14, 2022
aucampia added a commit that referenced this issue Jul 15, 2022
If `Store.bind` raises a `TypeError`, and the string conversion of this
TypeError contains `override`, then log a warning and call `Store.bind`
without an override.

This is done so that stores that do not accept `override` on
`Store.bind` still work, but at the cost of still having the bug that
was fixed by introducing the `override` parameter.

Also added a private flag which can be used to disable the fix entirely
and never use `override` when calling `Store.bind`.

- Fixes #1880
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant