Proposal create project coding standard for tryton

Organisation
1

Proposal to create a project coding standard

use of pylint, automatic code formatting with black, start adding type hints to core libs.

Props

  • improves code consistence and readability
  • avoids annoying editor warnings, “save time and mental energy for more important matters”
  • avoids nagging about formatting
  • pylint is a tools that helps preventing certain type of bugs bugs, suggesting improvements
  • helps current developers and to board in new developers

Cons

  • applying black, pylint will create possible merge and rebase issues for existing banches
  • discussion about allowed variables names pylint and pep8 vs internal coding style example FiscalYear = pool.get('account.fiscalyear') vs fiscal_year = pool.get('account.fiscalyear')

Tasks

  • discus a coding style to be shared across projects
  • publish aproved coding style at Tryton Documentation
  • create a pylintrc with the approved coding style
  • discus a time to apply black across all projects
  • add a # pylint: disable= at the beginning of each python file with the rules that are not implemented, to ease development and progressive implementation of the coding style
  • implement hooks to ensure that pylint and black are enforced at commit

Future

  • as of python 3.5 was introduced type hints would be helpful to introduce on core libs
2

You can check coding guidelines on: https://www.tryton.org/develop#coding-guidelines

3

This points have very few to do with the coding style.
The key points are: good naming and proper design. And they are not and cannot be managed by tools.

Well that depends on everyone editors.

As I’m doing most of the reviewing, I have very little comment about format. And most of the time reviewbot has already commented.

Indeed I find that the formatting comes automatically out of a well though design. And usually bad code has bad design (even if it was linted or whatever).

We have already flake8 run by reviewbot. I do not think we must change but improve it.

I do not see how if they have to setup more tools in order to start.

Indeed it is more about backport of fixes. But it would be a major problem.

I already though about that but I could not find a way to deal with dynamic class generation Tryton has.
Maybe with a hook that create stub files automatically out of all the modules.
But anyway, this is off topic about code style.

4

Also for now our style was designed with back-port of patches in mind. So it tries to minimize the number of line modified (e.g. fixed indentation depending on number of opening brackets).
But an automated formatting does not do that, it may decide to use a different style because some limits are reached (I think about list with 1 item per line for example).

5

Hello,

Since black is now officialy stable, I would like to re-open this discussion.

Just to be clear, I really do not like the black coding style (and I really like the current Tryton style). However, as the main reviewer in a company with dozens of developpers, eliminating the style issues from reviews would be invaluble.

Actually, since it is “just” formatting, wouldn’t it be possible to just apply it on all maintained branches?

Since black is the (AFAIK) only widely used python formatter, most python users will already know about it.

Ignoring E127 / E128 sometimes lets slip formattings that should not be

6

Me too and even more I find it makes reading more difficult.
So it is the end of discussion?

No because the problem is applying back-port.

I personally do not like where the current Python community is going with formatting, typing etc. They are removing the simplicity and beauty of Python and make it looks like other languages.

The main reason we do not follow that is it makes backport really harder.
Indeed it will be good if we could tune flake8 to enforce line break at opening parenthesis.

7

I fully agree with this.

8

I just made a test with black -l 79 -S. It is not that bad but the having closing bracket on a single line is quite annoying lose of space. I guess it may be good to also test yapf which seems to have more configuration options (and which may be adapted to our style).

But at the end I think this can be applied only if we have a way to ignore the commit from the blame command. I remember seeing a talk from Octobus guys showing such tool but I can not find it back (if I remember correctly, it was an experimental feature).

9

Here is the rationale behind Django’s adoption. All may not be relevant, but I think it is interesting, since they also had the same questions

10

In the django discussion they talk about darker which applies black only on the region that change.

I am puzzled on this idea, it solves the issue but it’s strange to only black part of the code.

11

It will need to be adapted for mercurial but it does not seem to complicated as mercurial can output git-like diff.

Indeed I like the idea for a smooth transition. We can expect that after 1-2 years, most of the code will have been formatted.

12

Indeed the hg annotate command has a --skip (EXPERIMENTAL) option.
Mozilla is using it with a custom command line that get revision from a file.
I found also this interesting way using pattern matching in commit message. It is probably slower than having a file with revision.
Any way we should probably see for the Heptapod migration to see what can be supported.

1 Like
13

For Javascript, we could use Prettier but it does not support mixing quote like we do in Python between “keyword” and “literal”.

14

I just found ssort, could serve to maintain a good order in the methods.

15

I am reviewing a lot of module code currently and so I see sometimes some pretty complicated domain.

Here’s what black does:

    tax = fields.Many2One(
        "account.tax",
        "Tax",
        domain=[
            (
                "company",
                "=",
                Eval("_parent_line", {})
                .get("_parent_move", {})
                .get("company", -1),
            ),
        ],
        depends={"line"},
    )

while my “ideal” code would look like this:

    tax = fields.Many2One(
        "account.tax", "Tax",
        domain=[
            ("company", "=",
                Eval("_parent_line", {})
                .get("_parent_move", {})
                .get("company", -1)),
        ],
        depends={"line"})

I do get that it’s impossible for black to understand what I want as it’s a matter of taste, nevertheless I find its version quite ugly.

Moreover in this example there is only on leaf in the domain, I wonder what happens when there is two / three complicated leaves.

16

It will clearly separate each clause which may be annoying from where we come but maybe it is not that bad for readability.

As counter argument, I think this kind of domain should not be written. It will be better to have a company Function field because it makes the domain working inside the One2Many’s but also outside. And cherry on the cake, it allows to right the proper multi-company rule.

What will be the output of common Tryton’s class?

17

After some try the output it is not very helpfull (to much oppinionated maybe). When I proposed I was thinking to use it in a way to avoid such reviews:

We usually extend ORM method at the end.

18

Yes that’s something that always bothers me.

My way of defining the methods would be:

  1. __new__ and __init__ (but we should never use them in Model)
  2. __register__ then __setup__
  3. default_ methods
  4. on_change_ and on_change_with_ methods
  5. function getters and setters
  6. ORM methods
  7. view_attributes
  8. workflow methods (and the helper methods used by those before them).
  9. other methods

Some choices are arbitrary, others are pretty obvious and some are good™ but I can’t explain why. And when extending I would respect the same order.

But of course, it’s difficult to always respect this or even remembering to do it. Hence I think that ssort could be a good idea (but I am afraid it’s even more difficult to do than format code because there is a lot of semantic there).

1 Like
19

I globally agree on this order, just wondering why ORM methods are this “low”.

I would say that since they may change things significantly and globally, they should rather be at the top (right after the __register__ / __setup__).

Also, I would love to find a way to visually group methods related to a single field (default / on_change / domain / order / getter / searcher / setter) together, but no great idea so far… Best solution was to use manual vim folds, but that is not universal enough.

20

Because it’s first the method / fields that define the model and then the methods that define its behaviour. And overriding the ORM m

But I don’t think it would be doable to define a standard that would work in all the situations. It’s more guidelines to take into account.