Proposal: Prettify code

[Eothred]

I have a proposal to make the code closer to PEP-8 standard. There are quite good tools these days to help you along, I typically set up pre-commit with black and flake8. This can then easily be added in e.g. Travis so that any merge request is automatically checked after..

This merge commit already contains a proposed first config for the pre-commit, as well as automatic code cleaning from the black tool. There are numerous proposals from flake8 as well (see the ignore-list in the config file), however those edits are more manual to fix. Hence I thought I'd ask if you think this makes sense before I start on the endeavour of going through them..

[Eothred]

Forgot to mention how to use it:

The command "pre-commit run --all" checks all files in the repository. This is typically what you add to travis essentially.

The command "pre-commit install" installs the pre-commit hook, which tests any python code you are trying to commit later on locally.

[Eothred]

I should also mention that the changes from black are typically breaking most pull requests so you want to do such changes at the right moment (when there are few big requests).

[awesomebytes]

I think this is a great effort, having the code follow PEP-8 so automated formatting can be applied (and even better if it can be checked before merging new work) eases contributions and improves the quality of the codebase, in the long run.

I do have a question/request. When someone creates a Pull Request which does not follow the formatting standards, how can we make it as friendly as possible for this PR to be accepted?

In other projects where I've contributed I've found this to be an issue when someone with good faith adds a contribution, but it's just not following the formatting standards. This person faces the need of following a set of steps to format the code which they may not be familiar with and may even need to install additional tools to do so (which in some cases it's impossible!). Usual ways to deal with this is having a documented (and usually strict, which does scare away contributors) way to contribute.

I wonder if there is a way to automate not only the check for formatting but also propose, automatically, changes to the PR so it follows PEP8. I haven't looked into it, but @Eothred you seem experienced with this topic.

Also thanks for working on improving this fantastic project.

[Eothred]

Thanks for your feedback.

black proposes auto-formatting if anything is out of style (that is what I put in one of the commits, I did not do any of that myself).

flake8 shows quite clearly what is wrong with each error, it will look something like shown below (an excerpt from remi, when I have taken out the list of ignores). This kind of output could be picked up from the Travis test if the contributor hasn't configured pre-commit locally. It is quite clear to a python developer what needs to be changed, so either a maintainer comes in and adds those fixes or the contributor reads the log from Travis and fixes them.

I am not aware of flake8 doing such fixes automatically, maybe there are other similar tools that do.

...
remi/server.py:32:1: F811 redefinition of unused 'socket' from line 26
remi/server.py:50:5: F401 'urllib.parse.unquote_to_bytes' imported but unused
remi/server.py:210:39: E711 comparison to None should be 'if cond is not None:'
remi/server.py:212:25: E711 comparison to None should be 'if cond is None:'
remi/server.py:214:12: E713 test for membership should be 'not in'
remi/server.py:257:97: E203 whitespace before ':'
remi/server.py:287:9: E741 ambiguous variable name 'l'
remi/server.py:289:32: E203 whitespace before ':'
remi/server.py:291:48: E203 whitespace before ':'
remi/server.py:292:24: E203 whitespace before ':'
remi/server.py:342:29: E711 comparison to None should be 'if cond is None:'
remi/server.py:344:16: E713 test for membership should be 'not in'
remi/server.py:434:51: W291 trailing whitespace
remi/server.py:578:17: F841 local variable 'ws' is assigned to but never used
remi/server.py:605:24: E713 test for membership should be 'not in'
remi/server.py:618:32: E203 whitespace before ':'
remi/server.py:626:12: E713 test for membership should be 'not in'
remi/server.py:716:42: W291 trailing whitespace
remi/server.py:717:57: W605 invalid escape sequence '\E'
remi/server.py:778:29: E711 comparison to None should be 'if cond is not None:'
remi/server.py:903:13: F841 local variable 'prev_handler' is assigned to but never used
remi/server.py:976:9: F841 local variable 's' is assigned to but never used
...

[awesomebytes]

My questions goes around... can we make the PR trigger a run of black, and if it actually needs to change stuff, somehow propose a commit on top of the PR to fix it? So contributors work can be used with minimal trouble for both the maintainer and the contributor?

Sorry if I'm not being too clear :)

[Eothred]

I'm not aware of that possibility no, but maybe it exist.

[Eothred]

So I get the sense that maybe not go for black as it is too strict, but flake8 could make sense? I can certainly see that point.

[dddomodossola]

I'm not an expert about flake8 and black. But I know the importance of PEP8 and I trust you, if you think that it is the case, I agree. The only think is that I would like to avoid class properties to be refactored as in the example above (if possible). Let me know how we can proceed ;-)

[awesomebytes]

My proposal would be:

• Make a list of files to prettify, prioritizing first files that are unaffected by Pull Requests or features that are currently being worked on.
• Start prettifying them but with a manual step in play. Like how I did before, autoformat the file with your tool of choice and then readjust the parts you don't like to something closer to your liking and autoformat again until you find your sweet spot (importantly, the last step must always be an autoformat, so it adheres to PEP8).
• @dddomodossola if you feel like it is a lot of work, do some of those by yourself (hopefully those files will contain a fair variance of code) so others can understand what style do you like and mark in the list you did at the start the files you'd like help with (or just say that help is welcome anywhere).

[Eothred]

I believe black goes beyond PEP8 to enforce a very strict style. I think there are many who find it too opinionated. I think the idea is to have the same style across any project you read. autopep8 is another alternative which is less strict, that could perhaps be a better choice to make the change less brutal?

The examples you mention will be reformatted with black regardless of how nicely we put it beforehand (I believe). black insists to split in one variable per line.

[dddomodossola]

Personally, in the jungle of code formatters (that however I never use) I prefer the way of black. One single code formatting, the right one. Of course this removes the personal styling (and this could be good the most of the times).

@Eothred I had no time to test your changes. Please explain me what is going to happen. With these changes I suppose that Travis will check PEP8 conformity of code, and than? Will return warnings or will autofix the code?

I would like to invite @MohamedAlFahim to this discussion. He promoted PEP8 adjustments short time ago.

I'm neutral 😀 . I love my formatting about class properties (because of compactness) but I know there must be PEP8 compliance.

[Eothred]

It is a bit up in the air how you decide to run this. The tool that is needed is pre-commit, which can be easily installed locally for any developer with a pip install pre-commit. Whenever pre-commit runs, it then creates a virtual environment to check the code, where it internally keeps the tools that are defined in the version they are defined (flake8, black and whatnot).

A contributor that has pre-commit on their system, runs pre-commit install in the repository after having cloned it, to set up their git hook. What happens then is that whenever you commit a python code, the pre-commit hook will automatically run the tool pre-commit on the edited files only, and disallow a commit (locally) of a source file that is not obeying the standards defined. For black it modifies the files and exit, but you then just re-run and it will pass the black test. For flake8 it exits and you see the messages of which lines to change (this is usually quite obvious).

As a side-note I find it unfortunate to name the tool pre-commit as this is also the name of the hook, so this sort of explanation becomes confusing for people not used to git hooks.

Travis may also run pre-commit, to check that any merge request is following the same standard.

So in principle you could set up Travis to run these checks, you could keep it only for the "advanced developers" to clean the code from time to time, you could encourage use but not enforce it.. You can decide what you think makes more sense considering circumstances of the project and what you wish to achieve.

My recommendation would be to add it to Travis, and then encourage but not enforce that all contributors set this up locally as it is quite easy for them to do. You could do this encouragement in a "how to contribute" section in the readme or another suitable location for example.

[Eothred]

"Will return warnings or will autofix the code?"

Sorry forgot to answer this particular part. To my knowledge Travis doesn't autofix the code, only reports back how it looks. I have been using gitlab-ci more so than travis so I am no expert in configuring Travis. Technically I am not sure how Travis could auto-fix the code, as that would require some travis-bot to have write access to the cloned repository I think?

[mcondarelli]

I would also suggest editors like PyCharm tat will flag all PEP8 inconsistencies (and offer a fix). This way You can incrementally weed put "formatting errors".

On 5/26/20 4:44 PM, Yngve Levinsen wrote:

"Will return warnings or will autofix the code?"

Sorry forgot to answer this particular part. To my knowledge Travis doesn't autofix the code, only reports back how it looks. I have been using gitlab-ci more so than travis so I am no expert in configuring Travis. Technically I am not sure how Travis could auto-fix the code, as that would require some travis-bot to have write access to the cloned repository I think?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dddomodossola/remi/pull/388#issuecomment-634070339, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGUTQMGDQQCKZA55V6Z2D3RTPIUZANCNFSM4NJTYFZA.

[dddomodossola]

@Eothred thank you a lot for the explanation.

@mcondarelli thank you for the suggestion. The problem with PyCharm is that it absorbs all the pc resources. Toooooo heavy

[Eothred]

OK, this took me a few trials to get right. I was pondering how to combine pre-commit and travis in the most sensible way. I see you already have some flake8 tests included (and started to separate which you consider important to not break).

One way would be to just add pre-commit to the pip install, and then add pre-commit as an additional command in the script.

However, I thought it could be split better than that. I know of a tool tox which essentially elevates the travis configuration to a separate file (which makes it easier if you want to use another CI in the future). It also allows you to more easily run these tests locally. This possibly makes sense I think.

With the latest commit I've configured a working proposal for tox+pre-commit+travis. I have set it up such that the existing coverage tests and flake8 tests are required to pass, while the new pre-commit is testet but allowed to fail (so you can see the report but travis still says it is fine with all the errors).

Some questions you might have:

This looks complicated??

• Possibly, I'm not convinced it is the optimal solution yet but it isn't too bad either. To the developer who doesn't care it is quite transparent, perhaps even more so than before. For example the flake8 test is now only run once, and separately from the coverage tests. So flake8 testing result will pass or fail separately to other issues.

Do I need to understand all of this?

• Don't think so. If you want to debug travis results you probably want to use tox locally, otherwise you don't need to know tox or travis exist at all. Travis runs automatically when someone sends a pull request. You might want to add information about pre-commit to a "contributor guideline" as suggested earlier, that's it I think.

How can I run a tox test locally?

• tox, as with pre-commit, installs with a pip install tox. You can then test a specific environment with the TOXENV variable, e.g. TOXENV=py38 tox or TOXENV=flake8 tox

Is pre-commit still needed?

• Not really, but I would personally still consider it I think. In principle you could add the same configuration to tox.ini, however pre-commit configuration is dead easy and for most contributors this is the part that should be of interest to actually use actively during development. It integrates more smoothly as a git hook (personal opinion possibly).

Where is it configured?

• .travis.yml has the setup for the travis environment, but as you see it is now quite minimal, just installing and executing tox in each case with the defined TOXENV.
• tox.ini has configuration of what dependencies and commands are needed for the various test blocks.
• .pre-commit.yml has the configuration for the full set of syntax tests.

[Eothred]

By the way, the coverage tests were already configured to "pass" even if there are failed tests? That was intentional?

[Eothred]

Changed the title of the PR, I would propose to pull it in this version. It does not make any dramatic changes to existing code, while people can start using the git hook and fix the code on the files they edit if they so wish, gradually improving the source.

Alternatively you can run yourself at some point the pre-commit run --all-files to autofix all (this would be that magic split second where you had no one with an active branch).

[dddomodossola]

Thank you a lot @Eothred !