flake8 max line length

[pedrobaeza]

As seen here (https://github.com/OCA/carrier-delivery/pull/19/files#r15347475), we are discussing maximum line length to check by flake8.

Length	Pros	Cons	Votes
80	official length	very limited especially for Odoo function signatures and columns	@eLBati, @pedrobaeza, @qtheuret, @gurneyalex, @nhomar, @StefanRijnhart, @sebastienbeau, @lepistone, @drdran, Mario Arias, José Antonio Mora, @gdgellatly, @guewen, @jgrandguillaume, @rvalyi
100	More lenient, works with function signatures	Need consensus to change	@bwrsandman, Óscar Alcalá
120	Current tests, less resistance	Too long, wraps even in github	Grahame Jordan

(with votes from mailing list: http://openerp-community.2306076.n4.nabble.com/Openerp-community-Coding-Guidlines-long-lines-td4643741.html)

[bwrsandman]

I think 80 should be a target to try for, but I don't think a longer line should cause errors.
That being said, 100 as a hard limit is acceptable.

[pedrobaeza]

The question is that we are doing PEP8 compliance for all repositories now, so we can set the max line as we want, because we are the ones we are going to deal with it. Then, respect this max line length is easier for future contributions. Dont you think?

[eLBati]

Here is the old thread: http://openerp-community.2306076.n4.nabble.com/Openerp-community-Coding-Guidlines-long-lines-td4643741.html

[dreispt]

Citing PEP8 :

Limit all lines to a maximum of 79 characters.
For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters.

And is also states:

Some teams strongly prefer a longer line length. For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the nominal line length from 80 to 100 characters (effectively increasing the maximum length to 99 characters), provided that comments and docstrings are still wrapped at 72 characters.

It has been discussed in the ML, and the opinions were very divided.
So there was no no clear strong preference for adopting the 99 char max length.

Then, the status quo is pure PEP8 79 chars.

[bwrsandman]

Then, the status quo is pure PEP8 79 chars.

But it was rarely enforced. As someone who has ported a lot of our code to pep8, I guarantee it.

I started travis scripts with line-length of 120 so there would be less resistance to adopting the tests, while still pointing out ridiculously horizontal code. We, however kept it as a variable with the intention of changing it when such a agreement of line length could be met.

The main problem in our case is column declaration, especially the help texts and most of our function signatures. The rest of the code is easy to keep under 80.

Here is a typical function signature in odoo:

    def onchange_currency_amount(self, cr, uid, ids, currency_amount, currency_id, context=None):

The line is 98 characters long. The variable names are not exceedingly long, but you can see how a well-styled function could still surpass this easily.

The question is, which is better style? That line or the broken up version:

    def onchange_currency_amount(
        self, cr, uid, ids, currency_amount, currency_id, context=None):

[dreispt]

I did my share of fixes in Launchpad for reviewers requiring PEP8, so my personal experience is that under OCA taht was expected. In fact, when migrating to v8 my post-OCA modules barely need any PEP8 fixing.

The 80 char limit may look aggressive when you're not used to it, but you quickly get to know how to handle it. And, IMO, in the end it does helps you to write better code.

[bwrsandman]

Also, the new API does help with the long lines problem

[bwrsandman]

@pedrobaeza would you mind making a table in the original post so we can know who votes for which length

[pedrobaeza]

Done. May I put in it mail list votes?

[bwrsandman]

Maybe, they should get notified if you tag them, so if they change their minds they should chime in here.

[pedrobaeza]

Updated, but I think it's very clear, don't you?

[dreispt]

The PEP8 line length limit should be imposed only for 8.0 branches.
For previous version, only changed lines should be checked, as @bwrsandman proposed for OCB.

[bwrsandman]

OCB is a different monster, because we have no control over the style of odoo and OCB tries not to steer away from it.

I don't think we should ever check code on diffs in OCA, it would put too much responsibility on devs trying to fix small things. We can create an environment where the style is uniform, we should do this.

[pedrobaeza]

I agree with @bwrsandman.

Regards.

[dreispt]

There's space for discussion there, but personally I don't see the point for accepting new v7 modules not PEP8 compliant, now that we have the tooling to manage that. And I can't see the trouble about adding a few extra breaklines on the lines you edit in smaller contributions.
But again, it's just my opinion.

[bwrsandman]

We don't* accept modules non compliant o.O

* With the exception in line lengths and a few other Odoo specific exceptions and repo not yet set-up with travis

We fix pep8 with the addition of travis files or a commit after, whether the limit is 80 or 120, we just make sure the tests are green so subsequent PRs follow the same style rules.

[dreispt]

@bwrsandman Not sure if I'm following you: you mean you agree checking PEP8 on diffs for pre-v8 branches?

[bwrsandman]

@dreispt I don't understand your last comment.

I don't agree on checking PEP8 diffs in any case where we have the power to fix pep8 on the entire projects.

What we have been doing on each project and on each branch is setup automated tests, then fix the style. Any PR is then tested as a whole. If PEP8 errors are in the PR, they are raised because you go from having 0 pep8 errors to having some. No diff required. There is nothing different with the pre-v8 stuff.

What I am saying is, on repositories which are set up, we do not accept PRs which do not follow OCA pep8 (which, for now has a relaxed line length rule).

[eLBati]

My proposal:

• let's set 80 as per [IMP] PEP8 line length #47
• for v8 branches (which are pretty empty for now) we only accept green PRs (PEP8 + tests)
• for v7 branches and previous we also accept red PRs only if tests are green and the added/modified code by the PR is PEP8 compliant

[bwrsandman]

@eLBati Why would we ever accept red PRs? All subsequent PRs will be red.

Also what you propose makes no sense:
If tests in v7 are previously green (as they already are for a majority of projects) and your PR is red, then it is introducing an error. If the code is PEP8 compliant, the tests should not be red and if they are, it is because it is introducing a unittest error.

As someone who has spent a great deal of time fixing pep8 on 7.0 and 6.1 branches, I find it quite outrageous to relax the code style practice on v7.

Pep8 diffs will only show errors on the modified lines, it will not report side effects on later lines, say if you change a variable name and forget to change it later in a function. The same is true about unused imports. People will find a way arond the tests. Not to mention, how are going to test the overall health of the projects (travis builds on branches, not on PRs).

Let it be known I am completely against using diff pep8 on OCA module, and accepting red PRs.

[eLBati]

Because of old (already present) non-PEP8 modules.
See for instance OCA/l10n-italy#20
Non-PEP8 code is very common on v7 OCA branches

I someone makes a PR in order to fix a v7 module or to add a module to a v7 branch, I would not force them/us to fix all that non-PEP8 code before merging that PR.
But I would force them to not introduce new non-PEP8 code.
So, I would merge that PR if tests are green and the new code is PEP8 compliant

[bwrsandman]

Why don't you fix the pep8 on l10n-italy instead of messing with the dynamic of the entire OCA umbrella?

Seriously, it's one PR that will have all the problems of a non-stable check.

This is what has been done to l10n-canada, server-tools, program, travel-mgmt, management-system, etc.

Non-PEP8 code should not be in OCA branches, the move to travis is to fix it across the board.

What you're proposing makes it (arguably) more conveinant for repos which aren't PEP8 compliant and less conveinant for repos which already are.

It is not sustainable and fixes a short term problem.

Just fix your v7 branches, you can take those modifications and put them to your v8 branches.

If you need help fixing pep8 in l10n-italy, I can do it for you, I've already fixed a couple dozen branches.

[dreispt]

I agree: why accept red PRs.
On the other hand, you can't force anyone to clean up someone else's PEP8, for v7 and v6. Some of these may be on "minimal life support" an people may not want to spend with them more time than needed .

So let me improve on @eLBati 's proposal:

• let's set 80 as per [IMP] PEP8 line length #47
• for v8 branches (which are pretty empty for now) we only accept green PRs (PEP8 + tests)
• for v7 branches and previous we only accept PRs if tests are green and the added/modified code by the PR is PEP8 compliant

The third point can be achieved by checking PEP8 only for diff "+" lines:

• A new module for v7 would be required to comply PEP8.
• A fix on an existing module would only require for the new lines to be PEP8.
• Is someone decides to fix PEP8 for a v7/v6 module, great! From there on also any edit would be required to be PEP8.

[bwrsandman]

My argument is: don't make that standard for v7 branches.

[eLBati]

On 07/25/2014 02:55 PM, Sandy wrote:

Why don't you fix the pep8 on l10n-italy instead of messing with the
dynamic of the entire OCA umbrella?

Seriously, it's one PR that will have all the problems of a non-stable
check.

Actually, the majority of OCA v7 (and previous) branches contains
non-PEP8 modules.

With my proposal, I intended to unload the work of contributors in order
to have tests as priority, without having to wait for PEP8 fixes.
Because if I have to choose between having tests+PEP8 and having only
tests and having none of them, I prefer to have only tests.

But, if we have enough contributors (or few contributors very active
like you 😉) who make repos become PEP8 compliant in a reasonable
time, I withdraw my proposal

If you need help fixing pep8 in l10n-italy, I can do it for you, I've
already fixed a couple dozen branches.

You would be very kind 🙏

[eLBati]

On 07/25/2014 03:28 PM, Daniel Reis wrote:

The third point can be achieved by checking PEP8 only for diff "+" lines:

• A new module for v7 would be required to comply PEP8.
• A fix on an existing module would only require for the new lines
  to be PEP8.
• Is someone decides to fix PEP8 for a v7/v6 module, great! From
  there on also any edit would be required to be PEP8.

That's what I meant too. Sorry if I was not clear

[bwrsandman]

I understood what you meant, that's what git diff | flake8 --diff does.
You can even add some flags to git to encompass as whole function or to just show modified lines (no context)

I just don't feel like the benefits outweigh the issues. Converting to pep8 should be our (maintainers) top priority after adding travis tests, it's part of the process. Those less used repos don't have travis set up, therefore there are no problems with failing tests. To add travis and not fix pep8 (and tests, for that matter) is, and should be seen as a job half done.

To implement this would hurt us in the long run when diff related issues start popping up.

[dreispt]

Converting to pep8 should be our (maintainers) top priority after adding travis tests.

That's at the heart of the issue: it's not.
That's true when porting projects to v8, but it's not when maintaining legacy code we're not sure will be ported to v8.

[bwrsandman]

What about failing tests, then? Should we abandon that? Because for as many module that are not pep8 compliant, there are almost as many which fail their own yml test, unittests and installs.

[dreispt]

Module tests are not affected - they keep being checked in any of the above scenarios.
We're discussing just the PEP8 checks.

[bwrsandman]

They have the same problem. When you add travis tests, you will have modules failing pep8 and modules failing tests. When adding .travis.yml you have to fix failing tests, why not pep8?

[dreispt]

Because failing tests mean code errors, PEP8 means not meeting a style guide.
The first is a must have; the second is a nice to have.

In principle you're right: everything should be made PEP8 compliant.
In practice resources are limited, and you must focus on what will bring you more results. People won't want to spend time fixing what's not broken.

[bwrsandman]

You can see the build details. There are three sections: Style, Install and Tests. As a reviewer, you should look at the details even when the tests are green, due to the abundance of false positives and false negatives.

It is clearly shown in the log if the style is causing the error.

Some people do spend time fixing the style, and for us, this is the best way to tell.
Once pep8 is fixed on all modules (and it will happen), we won't be having the discussion.
Resources are limited, you're right, but people are also on vacation and fixing pep8 does not take that long, it helps review code and fix up errors. It only has to be done once.

Instead of arguing about this, why don't we just fix what is broken, so we can at least say we follow our own guidelines?

[rvalyi]

Hello, I think it's cool to be green on PEP-8 in v8 and also be very strict with the extra modules we will accept.

As for 7.0, I think we should not blind ourselves wasting to much resource on PEP-8 (except may be for the core modules) as it's better to focus on v8 and conceptual things when we still have a few weeks to decide bckaward incompatible changes for the next 18 months.

[bwrsandman]

@rvalyi How do you vote? line length = 80, 100, 120 or other?
That is, after all what this issue is about. :P

[pedrobaeza]

In all the time you have passed discussing about this, you may convert a couple of repositories to full PEP8 compliant ;)

For my part, I consider v7 branches for being full PEP8 as a requirement, and I compromise to convert a lot of them. It's not too much work, so I think we can stop already this debate.

Regards.

[rvalyi]

@bwrsandman I vote 80 at least for v8 (new API helps) but I'm not bigot about it, so I don't want to annoy people with my preference here.

[pedrobaeza]

Please, let's make the merge #47 so that I can start making PEP8 some branches this weekend.

[elicoidal]

I agree.
It is difficult to make decision out of an exception and based on such
few feedback.

Eric Caudal

On 07/25/2014 08:55 PM, Sandy wrote:

Why don't you fix the pep8 on l10n-italy instead of messing with the
dynamic of the entire OCA umbrella?

Seriously, it's one PR that will have all the problems of a non-stable
check.

This is what has been done to l10n-canada, server-tools, program,
travel-mgmt, management-system, etc.

Non-PEP8 code should not be in OCA branches, the move to travis is to
fix it across the board.

What you're proposing makes it (arguably) more conveinant for repos
which aren't PEP8 compliant and less conveinant for repos which
already are.

It is not sustainable and fixes a short term problem.

Just fix your v7 branches, you can take those modifications and put
them to your v8 branches.

If you need help fixing pep8 in l10n-italy, I can do it for you, I've
already fixed a couple dozen branches.

—
Reply to this email directly or view it on GitHub
#46 (comment).

[gurneyalex]

for the record, the reason I originally set the line length limit in the code was to ease having green builds.
personnaly I'm all in favor of a 80char line length limit (but I'm not fond of the way some people break lines to achieve such lengths: the correct way to do this IMNSHO is to use intermediate variables, not funky line breaks... but then this becomes a matter of taste).

I'm in favor of 80char for v8. And to be pragmatic, I'm ready to accept a slightly relaxed rule for v7 (say 100), if there is a consensus that it is not realistic to require maintainers to do the work. I myself am in favor of doing that work (as a maintainer), because a clean code base and green builds is worth it.

[bwrsandman]

the correct way to do this IMNSHO is to use intermediate variables, not funky line breaks

100% with you on this. It's good style and it doesn't really impact performance

[bwrsandman]

The issue is solved, I am closing this.

[guewen]

I'm a bit late on the debate, but I noticed that the line length configured in the script is 80 but it should be 79 actually: https://github.com/OCA/maintainer-quality-tools/blob/master/travis/travis_run_flake8#L5

My personal opinion: I do support the 79 chars limit. But in some (rare) cases, I still think that a line a bit more long could be more readable, this is subjective and left to human judgment though. Tools such as flake8 will always rise the red flag when I would accept a line a bit longer if I find it more readable.

[dreispt]

Would it be any good to be able to set the line limit in the travis.yml file?

[gurneyalex]

On 12/08/2014 11:54, Daniel Reis wrote:

Would it be any good to be able to set the line limit in the
|travis.yml| file?

—
Reply to this email directly or view it on GitHub
#46 (comment).

I'd rather not to, the qa tools provided by this project reflect a
consensus about what is accepted and what is not among OCA projects. If
the consensus is 90 lines, let's change this globally. If the consensus
is different among various versions of the backend, let's reflect this
in the tools, but I'd really like to have one central place reflecting
the consensus.

Eg, I'd be in favor of raising the line length for version 6.1, because
I don't really feel like fixing this in this old version of the projects.

Alexandre Fayolle
Chef de Projet
Tel : + 33 (0)4 79 26 57 94

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac Cedex
http://www.camptocamp.com

[bwrsandman]

I second @gurneyalex