search

dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

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

Closed cmihai closed 5 years ago

cmihai commented 5 years ago

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 .

Gnappuraz commented 5 years ago

We are in 2019, 120 chars I think would fit even in most cellphones... utACK https://github.com/dtr-org/unit-e/pull/1052/commits/dbed8b955f587db72092a4be33abfd596b1a1f91

AM5800 commented 5 years ago

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

AM5800 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Also see issue https://github.com/dtr-org/unit-e-project/issues/76