Make graph and other methods chainable

[wssbck]

Proposed Changes

Makes certain graph methods, like remove or add chainable. This will make it possible to write more concise code involving multiple graph modifications, along the lines of:

g = Graph().add(...).addN(...).add(...).remove(...)

[nicholascar]

@wssbck this change might only need a few test changes for it to pass. I guess that, since most of the methods like add() have always returned None, this is what a bunch of tests look for but I don't suppose there's any reason why they must look for such an outcome. So returning self will just require those tests to be changed. Looks like there's only about 8 of those test anyway, so it won't be a big deal to alter.

Are you still keen to see this PR through?

[wssbck]

@nicholascar I would like to continue, yes. I got distracted by, well, life.

[wssbck]

@nicholascar I added returns to more methods and adjusted all the tests. I hope I have not missed anything. The build is passing now.

[nicholascar]

@wssbck thanks for updating this PR. I'm happy that it's now doing what you want it to do and that it's passing tests however I want to check one thing with my co-maintainers still:

@ashleysommer @white-gecko: this PR makes add() and .remove() return self and I have two questions for you:

1. can you think of any down-stream projects that would be negatively affected by this?
2. is there any performance penalty incurred by this passing back? I don't mean data in self - it's just a reference - but does passing it back, even if it's not used, cost any extra memory or processing? I assume no but need to check

[ashleysommer]

Good question regarding performance. I don't think it will change anything, but I'll do a couple of benchmarks and get a number to compare.

[ashleysommer]

Ok, I created this simple benchmark (its a simple python method benchmark, not an rdflib benchmark):
https://gist.github.com/ashleysommer/86a30e4b3b356198db058a7c6b0393e1

Conclusion: zero performance difference when returning self rather that not returning anything.

[wssbck]

@nicholascar this PR changes return values of several methods from implicit None to explicit object references. From a practical standpoint it shouldn't cause anyone issues unless their code, for some reason, relies on the implicit return of None. I really cannot come up with a reasonable scenario where it could happen, though. It would be like relying on the return value of print (which also returns None).

The practical effect is that these methods can now be chained, potentially making some downstream code more compact.

Since the return values are now explicit, they are easier to reason about. It is, to my experience, always better to try to explicitly return something related to (or convenient because of) the purpose of the function/method rather than rely on the default behaviour of the language.

[nicholascar]

Thanks @ashleysommer, I'll add you to formally review then and we are good to merge

[ashleysommer]

Looks good

[nicholascar]

Since @ashleysommer's comment has been addressed, I'm merging

[nicholascar]

@wssbck you might like to update the rdflib documentation files with a new PR to show chained use in action

[wssbck]

I will try to find time shortly, thanks for reviewing and merging!

[aucampia]

I was looking at fixing type checking, since I had some issues using 6.0.0 in typed python, and noticed that serialize will now sometimes return self, but it will be a bit inconsistent, as sometimes it will still return None. I think for serialize there is actually a good case to make that it should not return self, as the return value somewhat depends on the arguments.

Just a note, I will fix the typing to correspond with what things are now, and then hopefully with type checking enabled in CI issues like this will be easier to spot.

[nicholascar]

serialize will now sometimes return self

Doesn't serialize return a string/bytes, e.g. https://github.com/RDFLib/rdflib/blob/master/rdflib/graph.py#L1013 & https://github.com/RDFLib/rdflib/blob/master/rdflib/graph.py#L1033?

...as sometimes it will still return None

As above, when would serlialize() return None? Do you perhaps mean something like add()? That might return self/None and it was the focus of this PR more than serialize(). Just trying to make sure I understand the point!

...I will fix the typing to correspond with what things are now...

Great, please do and I'll happily assist

[aucampia]

@nicholascar Graph.serialize() used to return None when a destination is supplied (see here and here)

This change makes Graph.serialize() return self if a destination is supplied.

However if a destination is supplied and it is a network location serialize() warns and returns None (see here).

I am actually unsure if this make sense, maybe this should rather raise a ValueError or RuntimeException than return None or self. But I guess if it does not throw it makes most sense to return self, which is what I changed it to in #1407.

[nicholascar]

@aucampia ok, thanks for that explanation.

I think you're right: always return the same thing (self) and return an actual error, rather than print a warning, when actually encountering an error condition.

I think you've already made a PR for this! But you have the network file location error as a warning not an error??

[aucampia]

@nicholascar I only thought that throwing maybe better than logging a warning after I wrote the code for #1407. I updated the PR now to make Graph.serialize throw a ValueError instead of raise an exception if the destination paramter is not a local file.