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

Increase the max line length for pep8speaks to 120 characters #1052

Merged
merged 1 commit into from
May 3, 2019

Conversation

cmihai
Copy link
Member

@cmihai cmihai commented May 3, 2019

Based on the example config from https://pep8speaks.com/.

Rationale: The default PEP8 line length is sometimes not enough for expressivity. See also https://medium.com/@drb/pep-8-beautiful-code-and-the-tyranny-of-guidelines-f96499f5ac17 .

Signed-off-by: Mihai Ciumeica <mihai@thirdhash.com>
@cornelius cornelius added the repo Configuration of the repository itself label May 3, 2019
@Gnappuraz
Copy link
Member

We are in 2019, 120 chars I think would fit even in most cellphones... utACK dbed8b9

Copy link
Member

@scravy scravy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK dbed8b9

@AM5800
Copy link
Member

AM5800 commented May 3, 2019

ACK dbed8b9
Existing codebase does not really fit into 79 chars. So enforcing 79 chars will only make our life harder

Copy link
Member

@cornelius cornelius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a more flexible line limit is good from my point of view. I would still strive for 79 character max lines, but this often is hard to achieve, so always having a warning when it's a bit more doesn't feel too helpful to me. 120 characters sounds like a reasonable compromise which is also not uncommon to be used by other people.

@AM5800
Copy link
Member

AM5800 commented May 3, 2019

Although this PR already has 4 approvals, I would like to ask not to commit it immediately after travis finishes. Because this topic is very hot and I believe people should have some time to express their concerns. So let's merge it in the evening or something

@frolosofsky
Copy link
Member

My concern is not relevant to this PR, but I missed a point we started using this bot. I personally do not like such things because they spam into the comments with no real benefit, because their reports are not something we will discuss, and it converts comments from discussion to developer log or alike. In my opinion, the pep8 checks should go to the Travis linter phase instead.

@cornelius
Copy link
Member

My concern is not relevant to this PR, but I missed a point we started using this bot. I personally do not like such things because they spam into the comments with no real benefit, because their reports are not something we will discuss, and it converts comments from discussion to developer log or alike. In my opinion, the pep8 checks should go to the Travis linter phase instead.

What in my view works best is a tool which actually comments on the code where the issues are. Something like https://houndci.com/. This is more like a normal review discussion than the one comment where pep8speaks collects all issues. If it is set up in a way that it only comments on the style issues we actually want to fix, then the bot takes the burden of nitpicking, which I think is a real benefit.

@cmihai
Copy link
Member Author

cmihai commented May 3, 2019

In my opinion, the pep8 checks should go to the Travis linter phase instead.

That is already the case, but failing the lint check doesn't currently stop the build. I've created issue #1054 for it.

@scravy
Copy link
Member

scravy commented May 3, 2019

I added pep8speaks which is an App which this pull request targets the configuration for it.

We have been thinking about this for some time already. The idea is that linting in travis might fail the build but could happen in parallel instead. Ideally you would get a report like "unit tests pass", "functional tests pass", "linting fails". This also eases review.

The app is not a mandatory check but adds one comment which is continuously updated and it only targets the touched lines, which is quite nice and would require a lot of effort in travis config otherwise.

@cornelius
Copy link
Member

Also see issue dtr-org/unit-e-project#76

Copy link
Member

@kostyantyn kostyantyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK dbed8b9

If we keep this service, I think it's reasonable to have 120-character lines for python as this is how currently our tests try to adhere.

Copy link

@castarco castarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK dbed8b9

I do prefer 80 characters, but it's more important for me to have a limit that everyone respects, and it seems that having 80 characters could motivate people to disable this check... so 120 is OK for me.

@cmihai cmihai merged commit 886f0ab into dtr-org:master May 3, 2019
@cmihai cmihai deleted the pep8-settings branch May 3, 2019 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repo Configuration of the repository itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants