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

Removal of six & styling #1051

Merged
merged 6 commits into from
May 24, 2020
Merged

Removal of six & styling #1051

merged 6 commits into from
May 24, 2020

Conversation

nicholascar
Copy link
Member

This PR removes all references to the package six which handles PY2/PY3 differences since we are no longer catering for PY2.

All tests pass locally - I even ran the SPARQLUpdateStore tests!

I've also blacked all Python files to ensure consistent formatting and addressed all issues from flake8 testing except for broad exceptions & line length warnings.

No functionality changes.

@ashleysommer
Copy link
Contributor

How hard would it be to split these into two different PRs, one for six and one for black and flake8 styling?

@coveralls
Copy link

coveralls commented May 17, 2020

Coverage Status

Coverage increased (+0.06%) to 75.707% when pulling b72bf07 on kill-Py2 into 231bc6d on master.

@nicholascar
Copy link
Member Author

Well, they are split by commits, but why the worry, they are all tidy-up steps without any functional changes?

flake8 used to be run semi-regularly on the codebase so that's why I did that one.

@ashleysommer
Copy link
Contributor

No concern really, should be fine.

@nicholascar
Copy link
Member Author

All right @ashleysommer & @white-gecko: please review and, if this gets past, we can then look into all the functional change PRs properly.

There are lots of edits here but they are all the same thing, many times over, in many files, so you should be able to be happy with the pattern after a few checks...

self.depth -= 1

return True

def isValidList(self, l):
def isValidList(self, l_):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why wasn't just l OK here? (And if it isn't, the docsstring ought to correspond.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only because flake8 threw up a lot of warnings about possible variable ambiguity for l so there must be a higher-order variable l still within scope that this might clash with.

I suppose I should have thought o rebuild docstrings before putting in the PR. I'll try and do that and add to this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the flake8 error regarding l is because it can look like an uppercase i or a number 1. Its generally considered using a single letter variable named l is a bad thing. This solution works to suppress the error, and I think doesn't harm anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. In practise I think l is good enough (in spite of flake8), but following that general advice, I think lst would be better, or even if we could establish a naming pattern like rlist (and similarly one for types, like rtype). (Going for rdf_list and rdf_type may seem even better/clearer, unless it becomes unclear whether they would reference an object or the terms themselves (RDF.type and RDF.List).)

Just something to consider going further, nothing critical for this PR.

Copy link
Member

@niklasl niklasl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks like a solid job!

examples/film.py Outdated
@@ -35,7 +35,7 @@

from rdflib import BNode, ConjunctiveGraph, URIRef, Literal, Namespace, RDF
from rdflib.namespace import FOAF, DC
from six.moves import input
from builtins import input
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to import input from builtins. Anything in builtins is automatically imported into every file by default, you can just use input() without importing anything.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ashleysommer, I've removed that line.

@nicholascar
Copy link
Member Author

@white-gecko can you please review this?

@nicholascar nicholascar merged commit 9103720 into master May 24, 2020
@nicholascar nicholascar deleted the kill-Py2 branch May 24, 2020 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants