colin icon indicating copy to clipboard operation
colin copied to clipboard

Resolve contradictory contributing guidelines for line length

Open rnjudge opened this issue 4 years ago • 8 comments

The Colin contributing guide states the following:

- Please make sure that your code complies with PEP8.
- One line should not contain more than 100 characters.

This is contradictory, however, as the PEP8 guidelines for line length state that all line lengths should be kept to a maximum of 79 characters. This is especially distracting when trying to look through the code using a PEP8-adhering editor as there are many lines longer than 79 characters that cause linting errors in these editors when trying to save a file.

rnjudge avatar Jun 15 '20 22:06 rnjudge

I should also mention that I am happy to work on this issue if it is agreed upon that lines should be 79 characters or less.

rnjudge avatar Jun 15 '20 22:06 rnjudge

@rnjudge You are right, thanks for creating an issue!

To be honest, we are using black in other tools (uses 88 characters by default) and I am thinking if it does not make sense to switch to black when we want to change a lot of lines. Black can do the reformating for us and we are quite satisfied with that since it saves us a lot of styling discussions.

What do others think? @TomasTomecek @lslebodn @jpopelka @dhodovsk?

@rnjudge Do you still want to work on it even if we switch to black? It will require:

  • [ ] run the black on all the python files (should be only one command..;)
  • [ ] update docs
  • [ ] ideally, setup pre-commit (like we have e.g. here)

lachmanfrantisek avatar Jun 16 '20 10:06 lachmanfrantisek

no strong preference from my side

@lachmanfrantisek +1

TomasTomecek avatar Jun 16 '20 12:06 TomasTomecek

Just to clarify, use black with line length being 79 max? I would strongly push for adhering to the PEP8 standards for line length but willing to work on this either way ;)

rnjudge avatar Jun 16 '20 16:06 rnjudge

We are using black without configuration elsewhere (it does not have many options for purpose) and it works fine.

Yes, it can sometimes break PEP8, but

  • lowering the length can generate less readable code.
  • if we use black as is we don't need to discuss line lengths and leave that decision to them.
    • Personally, I didn't know the line-lengths in black before. The good-looking code was more important.

So to clarify: I would use black as is but if you strongly want to pick 79, I am not against.

Thanks in advance!

lachmanfrantisek avatar Jun 17 '20 06:06 lachmanfrantisek

Black with default configuration +1

While looking at our pre-commit configs in other projects I was wondering why we use flake8 with --max-line-length=100 (when Black's default line-length is 88). And it seems (I don't see it in Black's docs) that Black does not wrap strings so they (the lines) can be any length. But we probably don't want to have them longer than 100 so that's why we warn (flake8 is just a linter, not a formatter) when they are over 100. Or do we want to set the flake8's max-line-length to 88 as well (so that we have one number - the Black's default one)? I'm personally fine with it as it is now, but we might want to discuss this as a team on a meeting and get back to you.

jpopelka avatar Jun 17 '20 09:06 jpopelka

I'm personally fine with it as it is now, but we might want to discuss this as a team on a meeting and get back to you.

Sounds good. I will continue to monitor this issue for a consensus.

rnjudge avatar Jun 17 '20 15:06 rnjudge

Se we discussed this and decided we want to keep the practice from our other repos, i.e.

  • use black with the default configuration, i.e. with line-length 88
  • use flake8 with --max-line-length=100 so that strings are max 100 chars
  • update the contributing guidelines, to reflect that

Regarding black & flake8, I'd suggest using pre-commit as described here

jpopelka avatar Jun 23 '20 09:06 jpopelka