Code style standardisation for Seldon Core Python modules

[axsaucedo]

As discussed on the roadmap towards 1.0, one of the points which was suggested was to standardise (and keep standardised) the code style in the Python modules.

We had revisited the Python Black Formatter briefly, and there were some nuances that people disagreed on the style approach.

Black does appear to be quite popular, so may be worth going forward with the 1.0 approach. It has over 10k stars, and its opinionated non-config approach would fit quite well with the integrateion tests we have.

The only other python formatter that would be worth considering is YAPF (Yet another python formatter), which is a project by google, which seems to have also quite a lot of traction, and has been there for longer (which can be both a good and a bad thing).

For a more practical view of how each of the tools would change our codebase:

• Here's what the Black Formatter PR would look like
• And what the YAPF Formatter would look like

The next steps would be:

• Decide on which formatter to go forward with
• Run the formatter for the codebase (ensure PRs are all landed)
• Set up formatter checks in CI

[adriangonz]

Nice one @axsaucedo!

I quite like black's approach. In Javascript there is a similar alternative, prettier. It's also super-opinionated but thanks to that it has managed to solve most "style wars".

[axsaucedo]

/assign @cliveseldon @jklaise @axsaucedo

[ukclivecox]

@jklaise What does Alibi use and why?

[jklaise]

In alibi we follow PEP8 (enforced by flake8 linter), with a few exceptions, most notable being the 120 line length, although I feel this is too generous and needs to be brought down (black uses 90). I do think adopting black across the projects would be the easiest thing, there are some minor quirks (e.g. it may do weird splitting if you do lengthy inline comments), but nothing we couldn't overcome. @arnaudvl @gipster @alexcoca

[axsaucedo]

Awesome stuff - we currently only have #946 and #942 opened which affect the Python code, so it seems it could be a good time to do the change, and could be pretty cool to announce at the Seldon Community meeting on Thursday

[alexcoca]

I personally think making the line length shorter does not always make sense - if you have to indent 3 or 4 times and try and write a comprehension/another for loop, for example, you might end up with random line breaks which make the code not readable. I'd stick with the 120, personally.

[jklaise]

@alexcoca my suggestion mostly stems from a lot of the time not being able to have 2 editor windows side by side even on a decent resolution monitor...

[axsaucedo]

Yeah I definitely see it being annoying on having multiple indents, but I agree with @jklaise on that one point about not being able to have stuff side-by-side. Black Formatter does seem to have an option for line length (https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length), which is good. I think best would be to start with defaults to get going, and perhaps see how this goes within a month?

[alexcoca]

That being said, I propose then to optimise s.t. we max the number of characters per line s.t. you can fit two windows side by side and not a character less 🕺

[axsaucedo]

This was resolved by adding black + git-commit-hooks