code formatting guidelines

[FlorianLudwig]

Hey there,

i noticed that there is black in the requirements.dev.txt. Does it mean that all merge requests are supposed to be formatted with black? The current code in master is not fully formatted by black though, making it harder to run black since it results in unrelated changes.

[white-gecko]

With travis we execute flake8 which does some basic style checks. I think this should be the minimum requirement. So far I do not have experience with black.

[FlorianLudwig]

Hey @white-gecko,

flake8 uses pycodestyle to check code formatting, which does conflict with black in some cases. I would recommend to either remove black from the requirements.dev.txt and by that stop suggesting it as formatter or use it to check the code as well (black --check in travis).

Having a code formatter as guide instead of a checker makes it easier for everyone imho ... but there is a certain pain involved when it reformats the code someone formatted by hand and disagrees with the decision made by the formatter. In the end, formatting does not matter that much to argue to much about it.

[ashleysommer]

Hi @FlorianLudwig

Yes, we recommend that all PRs are formatted using Black. That is why it is in requirements.dev.txt. [See note below].

You mention:

The current code in master is not fully formatted by black though

Please refer to these commits:
2020-05-16 Black all python files
2020-05-25 Black and other fixes
2020-08-19 More black formatting
2020-08-27 Re-Run black

The whole codebase (well, just everything in the rdflib/ directory) has been Blacked 4 times since May.

I believe the inconsistencies are from merging very old PRs, this causes non-blacked code to come in, necessitating re-running Black occasionally on the whole codebase. Can you let us know which files you believe to be not fully black formatted in master now?

I also find sometimes when working on a small change within one file, when I run Black before I commit the change, Black will make a small change in an unrelated file. I don't understand why it does that, because the other files should already be blacked, but I do still occasionally see it happen.

Note, I say its recommended to format PRs in Black, but it is not required or mandatory yet. We will eventually add the black --check to Travis, to test and enforce on incoming PRs, however that is a big shift in mentality for a lot of long-term RDFLib contributors, and we don't want to do anything that could further discourage talented developers from making contributions, so we'll want to roll it out carefully and at the right time, (maybe after the release of 5.1.0 or v6.0.0).

[FlorianLudwig]

@ashleysommer

Can you let us know which files you believe to be not fully black formatted in master now?

reformatted /home/f.ludwig/projects/rdflib/rdflib/events.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/namespace.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/plugins/serializers/turtle.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/plugins/parsers/rdfxml.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/plugins/sparql/algebra.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/graph.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/plugins/stores/memory.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/plugins/stores/sparqlstore.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/plugins/stores/sleepycat.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/plugins/parsers/notation3.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/extras/infixowl.py
reformatted /home/f.ludwig/projects/rdflib/rdflib/term.py

Files outside of the rdflib/ path:

reformatted /home/f.ludwig/projects/rdflib/docs/conf.py
reformatted /home/f.ludwig/projects/rdflib/test/test_issue184.py
reformatted /home/f.ludwig/projects/rdflib/test/test_graph_formula.py
reformatted /home/f.ludwig/projects/rdflib/test/test_issue209.py
reformatted /home/f.ludwig/projects/rdflib/test/test_issue200.py
reformatted /home/f.ludwig/projects/rdflib/test/test_issue801.py
reformatted /home/f.ludwig/projects/rdflib/test/test_issue910.py
reformatted /home/f.ludwig/projects/rdflib/test/test_memory_store.py
reformatted /home/f.ludwig/projects/rdflib/test/test_issue381.py
reformatted /home/f.ludwig/projects/rdflib/test/test_auditable.py
reformatted /home/f.ludwig/projects/rdflib/test/test_graph.py
reformatted /home/f.ludwig/projects/rdflib/test/test_parse_file_guess_format.py
reformatted /home/f.ludwig/projects/rdflib/test/test_slice.py
reformatted /home/f.ludwig/projects/rdflib/test/test_sparql_agg_distinct.py
reformatted /home/f.ludwig/projects/rdflib/test/test_nquads.py
reformatted /home/f.ludwig/projects/rdflib/test/test_dawg.py
reformatted /home/f.ludwig/projects/rdflib/test/test_wide_python.py
reformatted /home/f.ludwig/projects/rdflib/test/test_nt_misc.py
reformatted /home/f.ludwig/projects/rdflib/test/test_csv2rdf.py
reformatted /home/f.ludwig/projects/rdflib/test/test_container.py
reformatted /home/f.ludwig/projects/rdflib/test/test_util.py

I also find sometimes when working on a small change within one file, when I run Black before I commit the change, Black will make a small change in an unrelated file. I don't understand why it does that, because the other files should already be blacked, but I do still occasionally see it happen.

black itself is deterministic, so running it again on the same input gives the same output. If the output changes there might have been changes by others that where run through black - or your black version or black config might have changed. black does not offer much in respect to configurability - the one that might be interesting is to set --target-version py36 (or whatever is the minimum python requirement for rdflib).

I say its recommended to format PRs in Black

I will update my PRs then :)

[FlorianLudwig]

@ashleysommer

Following up on:

[...] however that is a big shift in mentality for a lot of long-term RDFLib contributors, and we don't want to do anything that could further discourage talented developers from making contributions

Here some ideas on encourage new developers:

• Providing more up-to-date documentation for the library itself
• Having code guidelines and getting started (how to run tests, which linters to use etc.)
• Providing good and timely feedback on MRs - to some extend this can be automated via CI - so including black in the

Speaking of docs and getting started, here a question I got:

(maybe after the release of 5.1.0 or v6.0.0).

The changelog.md stats that 5.x is python 2 compatible and it looks like 5.x is not yet branched of all. Does that mean MRs must be python 2 compatible? The CI does only test against python 3 though.

PS I will volunteer to make a contribution.md where I put in all the stuff I came across while trying to get started with rdflib as a first step towards that :)

[FlorianLudwig]

Okay, I tried running the tests on the master branch to run with python2.7 - doesn't seem like it's supposed to run. Since f69df42 it's python3 only.

[ashleysommer]

@FlorianLudwig

You might not know, but we are actually new maintainers on this project. @nicholascar , @white-gecko and myself took over maintainership of the RDFLib project in February 2020.

We understand it is frustrating that the code repository is not of a modern standard, and is not what new developers expect in 2020. Please remember there is code and documentation in this repository from 2003 or before. There is code in this repository written by Tim Berners-Lee for the W3C RDF and Turtle standards. So while it is very old, it should be respected for what it is.

Our main tasks as new maintainers was to get v5.0.0 release-candidate out, create a 5.0.0 changelog, make a 4.x to 5.0 migration document, and finally publish v5.0.0 for everyone who was patiently waiting more than 4 years since the 4.2 release.
We were successful in doing that, and now we are starting to focus on other areas, but we are busy people.
We know there is work to do, and we are actively trying to work with the community to move the codebase forward without causing side-effects or breaking anything for our users.

Thanks for your advice:

• Providing more up-to-date documentation for the library itself

Yes, we know! At our last three maintainer meetings this has been on the list to discuss, and certainly needs to be done, and we know generally how to fix it. It just takes time.

• Having code guidelines and getting started (how to run tests, which linters to use etc.)

Looks like you've done that in #1161, thanks.

• Providing good and timely feedback on MRs

To be honest I think its pretty good already. Getting a reply within a week or two (on average) is much better than the 6 to 18 months average response that it used to be. We do try to give good feedback in code reviews to help contributors create high quality contributions.

Speaking of docs and getting started, here a question I got:

The changelog.md stats that 5.x is python 2 compatible and it looks like 5.x is not yet branched of all. Does that mean MRs must be python 2 compatible? The CI does only test against python 3 though.

I got a very similar question recently: #1115

RDFLib, like most code repositories, uses Trunk-based Development. That means all new features and changes go into master.

So just because the 5.0.0 release is python2 compliant, the same assumption cannot be made for master, because time has passed and development has been done in master. The first commit which started to remove Python2 support was 02c6c01 in April 2020.
See this comment on the other thread for more info: #1115 (comment)

[FlorianLudwig]

@ashleysommer Thanks for the explanations and background info - always helps to understand the context.

Only question left on my mind is: You were mentioning a 5.1.0 release - where would that be developed since master is 6.x?

[ashleysommer]

master is still labelled 5.0.0 (see here). However it should actually be changed to 5.1.0-dev or something along those lines. master represents work that will be released as v5.1.0.

But I do see now where your confusion comes from.

changelog.md stats that 5.x is python 2 compatible

Actually changelog.md shoud be correct to read "5.0.x is python2" compatible. After the release of v5.0.0 it was decided by a poll to no longer try to keep python2 compatibility, and everyone agreed to simply remove it. That is why 5.1.0 (and master) is not python2 compatible. However if we need to do any emergency 5.0.1 release, then it will be python2 compatible. The beauty of Git is you can go back in time to any point in the code's history and make a branch from there if needed.

I guess if it has caused this much confusion, that wording should be changed in changelog.md.

[nicholascar]

Yes, master should actually be changed to 5.1.0-dev or something along those lines, agreed!

[FlorianLudwig]

Oohh, yeah, I misunderstood that - and actually my merge request to update the README to clear it up now carries that misunderstanding with it.

I though 6.x was the version "to break stuff" - including python2 compatibility. The poll @ashleysommer mentioned also has @nicholascar commenting

Yes, keeping 5.x to bug fixes and moving master, the in-preparation 6.0.0 release, to Py3.5+ is definitely the plan.

Which I understood as master = 6.0 not 5.1. Which is now on the frontpage/readme:

https://github.com/RDFLib/rdflib#versions

[FlorianLudwig]

btw, does RDFLib strive to follow semantic versioning?

[ashleysommer]

Ok, yeah that PR should've been reviewed more thoroughly before making it to the frontpage readme.
5.0.x will be python2-compatible (if there is any need for this release)
5.1.x will be python3-only
While we originally planned to support as far back as Python v3.5 for compatibility reasons, @nicholascar and I have since decided we'd prefer to support only Python 3.6 and above for v5.1.x and v6.0.0.

[ashleysommer]

btw, does RDFLib strive to follow semantic versioning?

semver as a formal concept has only existed since 2011, this project was around for at least 8 years before that. As fast as I can tell, the versions of RDFLib in the past were bumped a major version when support for a new major version of python was added, or support for a old major version of python was dropped. I don't think it ever correlated with features being added or removed, or breaking changes.

We'd like to start to implement semantic versioning going forward. While our current versions try to communicate meaning to an extent, we are not yet actively doing semantic versioning.

To do it properly requires a lot more coordination and planning put into the repository in general. More extensive unit tests, more integrated integration tests, deeper understanding of our user's use-cases.
We'd need to start doing more frequent incremental releases in order to fully take advantage of the semantic nature of the versioning.
This ties into the need to generate more detailed changelogs, and upgrade guides to further communicate to users where breaking changes may occur.

[FlorianLudwig]

If going forward semver is an interesting concept, I'd say the current master should be 6.x as otherwise it would be breaking changes in a minor release.

[ashleysommer]

@FlorianLudwig
Revisiting the differences we were seeing with the Black tool.

After the latest round of Pull-request merges today, I've manually run black ./rdflib and needed to make some small formatting changes to just two files (namespace.py, and parser.py). These changes were required as a result of merging the pull-requests today.

Now when I run black, I see:

> black --version
black, version 19.10b0

> black ./rdflib
All done! ✨ 🍰 ✨
75 files left unchanged.

Do I understand that you are seeing a different result when you run black?

[FlorianLudwig]

I think this also solves #1159, or at least interferes with is, as this decides to take out black and use nose.
Originally posted by @white-gecko in #1175 (comment)

But nose and black don't have overlapping purpose how can one be replaced by the other?

[white-gecko]

Ah yes, you are right. I mixed something up here.