Coding Style Guide

[Jonas Obrist]

Hi everyone,

One topic that came up in the last core developer meeting was coding style (mostly focused on line length which is a subset of it), so I thought let's bring it up here for discussion.

First of all, let me make it clear that it is not my intention to blame anyone or accuse them of bad code style. Quite a bit of code in the codebase is by me or was merged/reviewed by me, further in the absence of a coding style guide, there's no way of breaking it. Secondly, this is all just my opinion. I will for ease of reading of this post not repeat "in my opinion" everywhere, so it might at times sound like I make absolute statements, which is not my intention.

Before I go into what I'd propose as a style guide, I should probably explain why I think it's a good idea to have an (enforced) style guide:

* It makes code a lot easier to read. (Imagine a novel where each paragraph is written by an author of a different writing style)

* Which makes it more maintainable. (Code is written once, read many many times)

* It can make bugs more obvious, as they tend to break the "pattern" the rest of the code follows. (Example context.update which should be prohibited/strongly discouraged by a style guide) 

* It makes the codebase appear more professional.

* It eliminates the discussion about line length!

At my current job, we have an enforced style guide for all Python code. At first, this was a pain to get into but after about a week you get used to it, have your editor configured to do most of the work for you and you start to reap the benefits. There's tools in Python to check style guides (as long as they mostly follow PEP8 which seems a good starting point) such as pychecker, pyflakes, pep8 and others, many of which integrate nicely into IDEs and editors and can help a lot to follow them or in some cases even do it for you.

I'd suggest we follow the guide enforced by the pyflakes and pep8 command line tools. Obviously migrations would be ignored. Other than that I'd say be as strict as possible with as little use of # nopep8 as possible.

Obviously we have a lot of code lying around, so if we go this way the question of "how?" will come up quickly. I suggest that we would start by making sure all new code complies with the new style. Then port file by file over (yes I'm volunteering to do most of the work here, assuming it's a style I can agree with). The reason not to do it in one big PR is that this would take some time, during which the file changes which would cause unneccessary extra merge work. During this period, have a PR open against a branch [1] that has tests for the coding style and that will eventually hit green, at which point it can be merged and used to ensure we don't slip.

- Jonas

[1] https://github.com/ojii/django-cms/compare/pyflakes-tests

[Peter J. Farrell]

This message isn't meant to be crass... More observational. This is an interesting post, however it seems to be a rather exclusive club as it sounds like a lot of discussion about those open source project is done behind closed doors. I've posted a few messages on this list in the past because I'm interested to contribute to the project, but no one responds. I've tried the IRC channel as well. Is there interest in getting more help from the community? I mention this because it seems hard to contribute from an outsider to the core team.

[Daniele Procida]

On Thu, Dec 4, 2014, Peter J. Farrell <maest...@gmail.com> wrote:

>This message isn't meant to be crass... More observational. This is an
>interesting post, however it seems to be a rather exclusive club as it
>sounds like a lot of discussion about those open source project is done
>behind closed doors.

Hi Peter. I completely understand that feeling.

We're aware that it's something that needs to be addressed, and we're making efforts to address it. See <https://www.django-cms.org/en/blog/2014/11/06/development-django-cms/>, for example.

There are very few things that go on "behind closed doors": we have occasional core team video conferences, and even rarer technical board meetings. We also have a core team email list, though it's mainly used to arrange meetings.

Everything else that we do is public, on IRC, email, GitHub and so on, and you are very welcome to take part.

> I've posted a few messages on this list in the past
>because I'm interested to contribute to the project, but no one
>responds. I've tried the IRC channel as well. Is there interest in
>getting more help from the community? I mention this because it seems
>hard to contribute from an outsider to the core team.

Sorry about that. We really *do* want your involvement. I'm evildmp on the IRC channel; if you ping me there I will always respond, if I'm around. Be aware that most of us are in western European time zones.

Reagrds,

Daniele

[Daniele Procida]

On Thu, Dec 4, 2014, Jonas Obrist <ojii...@gmail.com> wrote:

>Before I go into what I'd propose as a style guide, I should probably
>explain why I think it's a good idea to have an (enforced) style guide:

I agree we should have a style guide, and enforce it.

I think I'd prefer a slightly longer line length than you do - say, 100 columns instead of 79.

Daniele

[Iacopo Spalletti]

In data giovedì 4 dicembre 2014 08:47:40, Jonas Obrist ha scritto:
> Hi everyone,
>
> One topic that came up in the last core developer meeting was coding style
> (mostly focused on line length which is a subset of it), so I thought let's
> bring it up here for discussion.
>

I'm strongly +1 on your proposal and having strict checks for it
I remember a style guide proposal on the wiki but I can't find it anymore :/

I have a couple of remarks on a style guide:
- line length: I'd opt of 100 chars, as it makes django code much more
readable
- import style: I'm decidedly against having one import per line when
importing multiple symbols from the same package:

-1
from django.contib.admin ModelAdmin
from django.contib.admin TabularInline
from django.contib.admin StackedInline

+1
from django.contib.admin (ModelAdmin, TabularInline, StackedInline)

The first form is more diff-friendly, but I think it's (uselessly) more
verbose and error prone than the latter

my 2 cents
--
Cheers
Iacopo Spalletti

[Jonas Obrist]

On Tuesday, December 9, 2014 2:49:47 AM UTC+9, yakky wrote:

In data giovedì 4 dicembre 2014 08:47:40, Jonas Obrist ha scritto:
> Hi everyone,
>
> One topic that came up in the last core developer meeting was coding style
> (mostly focused on line length which is a subset of it), so I thought let's
> bring it up here for discussion.
>
I'm strongly +1 on your proposal and having strict checks for it
I remember a style guide proposal on the wiki but I can't find it anymore :/

I have a couple of remarks on a style guide:
- line length: I'd opt of 100 chars, as it makes django code much more
readable

I don't see why Django code is "longer". Regardless, one argument one can make for 79 char limit is that PEP8 actually suggest that. A very lazy (but in my opinion still valid) argument is also that existing linters such as pep8 expect a 79 char limit, meaning implementing the automated check will be more difficult if we go for a "custom" line length, as opposed to the "standard" (among those projects that use a line length limit). I found it surprisingly easy to switch to 79 chars/line and only have to use #nopep8 extremely rarely (mostly for imports as we use deeply nested modules with fairly long names).

 

- import style: I'm decidedly against having one import per line when
importing multiple symbols from the same package:

-1
from django.contib.admin ModelAdmin
from django.contib.admin TabularInline
from django.contib.admin StackedInline

+1
from django.contib.admin (ModelAdmin, TabularInline, StackedInline)

The first form is more diff-friendly, but I think it's (uselessly) more
verbose and error prone than the latter

Could you explain how the more verbose version is more error prone?

 

my 2 cents
--
Cheers
Iacopo Spalletti

- Jonas 

[Tim Graham]

For what it's worth, Django recently cleaned up its code to conform to most PEP8 rules. Automated checking (we have Jenkins fail a pull request build if there are any errors) has been a big win, so that reviewers don't have to worry about those details. We are using flake8 and have some rules ignored in setup.cfg that we deem are more trouble than its worth to cleanup as well as max-line-length = 119 (the width of GitHub's review pane). I started by adding all of the current error types to the flake8 ignore list and then created a ticket for each type of error we wanted to remove. These tickets ended up being a popular item for new contributors and its fairly easy to merge since the "test" is that removing the error from flake8 exclude doesn't introduce any new errors. We are also looking at using something like isort to automate import ordering so its one less thing to think about (see https://code.djangoproject.com/ticket/23860). Hope that helps.

[John-Scott]

Thank you for your input Tim!

I don't have much influence, but I'd love to see django-cms participate more directly in the wider Django community, adapting Django conventions and best practices rather than reinvent the wheel.

• django-cms's coding conventions should follow Django's lead
• If there are compelling reasons not to, these should be discussed upstream first to see if we can come to a common solution
• Contributors (who can be assumed to be familiar with Django) shouldn't have to learn idiosyncratic syntax rules for django-cms but should instead count on the coding style docs for Django to be standard.

That said, I'd love to see as much passion focused on the long list of open tickets as well as the requests for help from potential contributors on the mailing list before we get into potentially pointless style debates. I have contributed several fixes and documentation updates for Django 1.7 and Python 3 support over the last couple of months (I'm jsma on Github), which I only mention so I'm not dismissed as a lurker/whiner.

There have been many significant changes in the 3.0 release and subsequent 3.0.x branch but not as much in the way of coordinated effort to make sure the test suite actually covers the code effectively (so much of the user experience is dependent on front-end editing now yet the test suite sometimes completely bypasses this and opts for the easier to test but insufficient API to 'exercise' the code). Frankly, django-cms has some serious needs at the moment but line lengths of 79 vs whatever number do not appear to be among them.

Sorry for hijacking the thread. I do sincerely appreciate everyone's efforts (special thanks to EvilDMP, yakky and mkoistinen for their help recently on IRC and Github). I continue to do my best to contribute back and I will also gladly contribute beers if any of us meet IRL (I'll be at PyCon 2015 in Montreal, so take me up on this offer if you'll be there or if you'll be in NYC anytime soon). But as a contributor, I have to admit I'm frustrated that this is the most active thread on the list recently when there are other Github tickets/mailing-list threads clearly asking for help solving actual bugs which break actual sites.

To get back on topic, +1 to following Django's lead on coding styles. But a bigger +1 on making sure the code actually works as advertised. On the later point, let's coordinate some sprints to get the backlog addressed.

Please and thank you!

John-Scott

[Iacopo Spalletti]

In data lunedì 8 dicembre 2014 10:16:07, Jonas Obrist ha scritto:
> On Tuesday, December 9, 2014 2:49:47 AM UTC+9, yakky wrote:
> > In data giovedì 4 dicembre 2014 08:47:40, Jonas Obrist ha scritto:
> > > Hi everyone,
> > >
> > > One topic that came up in the last core developer meeting was coding
> >
> > style
> >
> > > (mostly focused on line length which is a subset of it), so I thought
> >
> > let's
> >
> > > bring it up here for discussion.
> >
> > I'm strongly +1 on your proposal and having strict checks for it
> > I remember a style guide proposal on the wiki but I can't find it anymore
> >
> > :/
> >
> > I have a couple of remarks on a style guide:
> > - line length: I'd opt of 100 chars, as it makes django code much more
> > readable
>
> I don't see why Django code is "longer". Regardless, one argument one can
> make for 79 char limit is that PEP8 actually suggest that. A very lazy (but
> in my opinion still valid) argument is also that existing linters such as
> pep8 expect a 79 char limit, meaning implementing the automated check will
> be more difficult if we go for a "custom" line length, as opposed to the
> "standard" (among those projects that use a line length limit). I found it
> surprisingly easy to switch to 79 chars/line and only have to use #nopep8
> extremely rarely (mostly for imports as we use deeply nested modules with
> fairly long names).

Django code, as most OOP-style one, tends to be "longer" than regular python
and quite often you'll find indenting in the last few chars before the 79-cols
limit, or breaking the calls unnaturally in the middle of a call chain; 99-
cols allows much more flexibility.

>
> > - import style: I'm decidedly against having one import per line when
> > importing multiple symbols from the same package:
> >
> > -1
> > from django.contib.admin ModelAdmin
> > from django.contib.admin TabularInline
> > from django.contib.admin StackedInline
> >
> > +1
> > from django.contib.admin (ModelAdmin, TabularInline, StackedInline)
> >
> > The first form is more diff-friendly, but I think it's (uselessly) more
> > verbose and error prone than the latter
>
> Could you explain how the more verbose version is more error prone?
>

More code, more errors :)
And anyway, why repeating the same import prefix over and over? What's the
gain?

As for the John-Scott proposal, Jonas proposal is quite adherent to the Django
coding style and we are in a good position to keep the two project synced,
even if constraints and requirements are very different between the two
projects, so we may have the need for some degree of flexibility.

--
Ciao
IS

[Jonas Obrist]

The Django Code Style is too lenient for my taste, but a good start. It's basically PEP8 minus line length plus conventions about models (which I like). I'm not as hung up on the number 79 as it might seem, so if the only issue here is that it should be 99, having a clear coding convention is more important to me than making PEP8 happy.

To address John-Scotts point of there being more important stuff, yes maybe that's true. But I don't suggest we drop everything and just make the codebase adhere to a style. The idea is to define a style, and then start moving the codebase there.

[Iacopo Spalletti]

In data martedì 9 dicembre 2014 02:51:30, Jonas Obrist ha scritto:
> The Django Code Style is too lenient for my taste, but a good start. It's
> basically PEP8 minus line length plus conventions about models (which I
> like). I'm not as hung up on the number 79 as it might seem, so if the only
> issue here is that it should be 99, having a clear coding convention is
> more important to me than making PEP8 happy.

\O/ :D

>
> To address John-Scotts point of there being more important stuff, yes maybe
> that's true. But I don't suggest we drop everything and just make the
> codebase adhere to a style. The idea is to define a style, and then start
> moving the codebase there.

Actually porting code to a (well-known) defined code style will make easier to
contribute.
Sure we have a backlog of issues to tackle, and I'd more than happy to promote
a sprint to coordinate the efforts and bring the community more closer
together.

@Martin: how about raising the idea during the next Hangout?

Iacopo Spalletti

PGP key block: http://www.spalletti.it/pgp