Make graph and other methods chainable by wssbck · Pull Request #1394 · RDFLib/rdflib (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation19 Commits6 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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(...)
@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?
@nicholascar I would like to continue, yes. I got distracted by, well, life.
@nicholascar I added returns to more methods and adjusted all the tests. I hope I have not missed anything. The build is passing now.
Make graph methods chainable Make graph and other methods chainable
@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:
- can you think of any down-stream projects that would be negatively affected by this?
- 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
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.
@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.
Thanks @ashleysommer, I'll add you to formally review then and we are good to merge
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
@wssbck you might like to update the rdflib documentation files with a new PR to show chained use in action
I will try to find time shortly, thanks for reviewing and merging!
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.
aucampia added a commit to aucampia/rdflib that referenced this pull request
Other changes:
- fix return value for
Graph.serialize(refs RDFLib#1394) - remove default value for
rdflib.plugins.sparql.algebra.translateAlgebra(refs RDFLib#1322) - add .dockerignore to reduce context size and make docker quicker to run.
- add .flake8 config to ignore line length as black is managing formatting.
- add mypy to docker-compose, makefile and tox.ini
- fix the way csv2rdf is invoked to ensure that the right code gets executed.
aucampia added a commit to aucampia/rdflib that referenced this pull request
Other changes:
- fix return value for
Graph.serialize(refs RDFLib#1394) - remove default value for
rdflib.plugins.sparql.algebra.translateAlgebra(refs RDFLib#1322) - add .dockerignore to reduce context size and make docker quicker to run.
- add .flake8 config to ignore line length as black is managing formatting.
- add mypy to docker-compose, makefile and tox.ini
- fix the way csv2rdf is invoked to ensure that the right code gets executed.
aucampia added a commit to aucampia/rdflib that referenced this pull request
Other changes:
- fix return value for
Graph.serialize(refs RDFLib#1394) - remove default value for
rdflib.plugins.sparql.algebra.translateAlgebra(refs RDFLib#1322) - add .dockerignore to reduce context size and make docker quicker to run.
- add .flake8 config to ignore line length as black is managing formatting.
- add mypy to docker-compose, makefile and tox.ini
- fix the way csv2rdf is invoked to ensure that the right code gets executed.
aucampia added a commit to aucampia/rdflib that referenced this pull request
Fixes RDFLib#1311
Add mypy to .drone.yml and fix type errors that come up.
Not type checking examples or tests.
Other changes:
- fix return value for
Graph.serialize(refs RDFLib#1394) - remove default value for
rdflib.plugins.sparql.algebra.translateAlgebra(refs RDFLib#1322) - add .dockerignore to reduce context size and make docker quicker to run.
- add .flake8 config to ignore line length as black is managing formatting.
- add mypy to docker-compose, makefile and tox.ini
- fix the way csv2rdf is invoked to ensure that the right code gets executed.
@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.
@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??
@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.