Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve contradictory contributing guidelines for line length #240

Open
rnjudge opened this issue Jun 15, 2020 · 8 comments
Open

Resolve contradictory contributing guidelines for line length #240

rnjudge opened this issue Jun 15, 2020 · 8 comments
Labels
documentation Documentation needs updates refactor Technical debt - the code needs love.

Comments

@rnjudge
Copy link
Contributor

rnjudge commented Jun 15, 2020

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 rnjudge changed the title Resolve contradictory contributing guidelines Resolve contradictory contributing guidelines for line length Jun 15, 2020
@rnjudge
Copy link
Contributor Author

rnjudge commented Jun 15, 2020

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.

@lachmanfrantisek
Copy link
Member

@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)

@TomasTomecek
Copy link
Member

no strong preference from my side

@lachmanfrantisek +1

@rnjudge
Copy link
Contributor Author

rnjudge commented Jun 16, 2020

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 ;)

@lachmanfrantisek
Copy link
Member

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 lachmanfrantisek added refactor Technical debt - the code needs love. documentation Documentation needs updates labels Jun 17, 2020
@jpopelka
Copy link
Member

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.

@rnjudge
Copy link
Contributor Author

rnjudge commented Jun 17, 2020

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.

@jpopelka
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation needs updates refactor Technical debt - the code needs love.
Projects
None yet
Development

No branches or pull requests

4 participants