-
Notifications
You must be signed in to change notification settings - Fork 2
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
Retain literal types in collection backend #165
Retain literal types in collection backend #165
Conversation
…by the collection backend.
…collection-backend
`fallback_backend`.
…collection-backend
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #165 +/- ##
==========================================
+ Coverage 75.94% 76.82% +0.87%
==========================================
Files 17 17
Lines 1534 1536 +2
==========================================
+ Hits 1165 1180 +15
+ Misses 369 356 -13 ☔ View full report in Codecov by Sentry. |
fallback_backend: If the current backend doesn't implement | ||
parse, use the `fallback_backend` instead. | ||
fallback_backend_kwargs: Dict with additional keyword arguments | ||
for initialising `fallback_backend`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see where these are tested. According to codecov, they are, so currently it should be OK. But it is important that this is tested on purpose and not "by accident", e.g. by using a backend that right now does not contain parse but might in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are tested in tests/backends/test_collection.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I prefer tests to be a bit more explicit.
fallback_backend: If the current backend doesn't implement | ||
serialisation, use the `fallback_backend` instead. | ||
fallback_backend_kwargs: Dict with additional keyword arguments | ||
for initialising `fallback_backend`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for parse above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in tests/backends/test_collection.py
tripper/backends/collection.py
Outdated
# Possible bug in mypy | ||
# parse_object() is declared to return "Union[str, Literal]". | ||
# Dispite of that complains mypy about that `v` has no | ||
# attribute "value" | ||
obj = v if isinstance(v, str) else str(v.value) # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Possible bug in mypy | |
# parse_object() is declared to return "Union[str, Literal]". | |
# Dispite of that complains mypy about that `v` has no | |
# attribute "value" | |
obj = v if isinstance(v, str) else str(v.value) # type: ignore | |
obj = v if isinstance(v, str) else str(v.value) |
I have thested locally and with a draft pull request, I do not get any errors with mypy. Do you have an updated version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem came when I updated mypy. I got annoyed with it and changed the return type of utils.parse_object() from Union[str, Literal]
to Union[str, Any]
to get rid of these errors...
The source of the problem is that mypy suddenly has got the idea that Literal
has got a special meaning, since it was added to typing
.
It even doesn't understand if I change the return type of utils.parse_object() to Union[str, tripper.Literal]
.
Using Any
in the return works around the problem. I have moved and updated the comment about buggy mypy to parse_object() and removed the # type: ignore
as you suggested.
fallback_backend="rdflib", | ||
fallback_backend_kwargs=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we didn't need to change backend/backend_kwargs to fallback_backend/fallback_backend_kwargs in the tests, it seems that we do not have adeguate tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the collection backend does not provide a parse() method, so it will use the fallback backend. That is tested when calling ts.parse() in tests/backends/test_collection.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems OK, although I do not understand the mypy issue.
Description:
Closes #160
Also added
fallback_backend
option toTriplestore.parse()
andTriplestore.serialize()
to allow calling parse() and serialize() with the collection backend (using rdflib under the hood).Note that this PR builds on top of PR #161
Note also that this PR utilises DLite PR SINTEF/dlite#755. It is probably a good idea to merge that PR and create a new release of DLite before merging this PR to master.
Type of change:
Checklist for the reviewer:
This checklist should be used as a help for the reviewer.