-
Notifications
You must be signed in to change notification settings - Fork 560
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
Update graph operator overloading for subclasses #1349
Update graph operator overloading for subclasses #1349
Conversation
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.
Looks sensible to me.
The only test failure is an address assignment, so not a real one.
@shreyasnagare thinks for this PR - looks good to me. Can you please add in your "steps to reproduce" as an actual test though? So test_graph_operator.py or similar? Thanks. |
@shreyasnagare thanks for that rapid turn-around! Just waiting for another review from one of my co-maintainers and then we'll likely merge. Please do feel free to make more good updates like this! |
Great to see this change, and thanks @shreyasnagare for finding and addressing it. I'd like to incorporate these changes in the |
@gtfierro 6.0.0 is only a couple of days away! We merged in the JSON-LD plug-in, sorted out namespace issues and merged in heaps of smaller PRs like this. Even updated docco. Just waiting for some final CI/CD steps and we will have the package into PyPI. Again, only a few days! |
Great news! Congrats on the release |
This PR makes the set operations for rdflib.Graph subclasses (like
g1 = g1 + g2
) more intuitive and similar to the results of in-place set operations (likeg1 += g2
).Example:
If you were to do a union of a
ConjunctiveGraph
(a subclass of Graph) instance with itself or any otherConjunctiveGraph
instance using something likeg = g + g
, the instance would lose all the methods specific to theConjunctiveGraph
.Steps to reproduce the issue:
Proposed Changes
type(self)()
instead ofGraph()
as the initial return value for set operations.Graph
to pass down similar behavior to its derived classes (unless overridden) without breaking any existing functionality.