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

[Proposal] Introduce style enforcement #61

Merged
merged 1 commit into from Nov 28, 2020

Conversation

MaLiN2223
Copy link
Contributor

@MaLiN2223 MaLiN2223 commented Nov 26, 2020

From my experience it is important (if not vital) to keep code style consistent across the whole codebase. Consistency improves making assumptions about how things should work but also improves readability (and as we know - readability counts).

Currently, the repository has a mandatory black step on travis but this is something that happens in the background during CI and it seems that the black changes are not committed to the main branch (please correct me if I'm wrong here). Locally running black gives me 142 files would be reformatted, 27 files would be left unchanged..

With this PR I would like to introduce enforcing a code style dictated (mostly) by PEP (through flake8) and black with small modifications and hook to pre-commit. This combination offers a quite easy transition and allows developers not to think/remember about formatting their code according to the guidelines.

Small explanations:
flake8 is a great toolkit for checking your code base against coding style (PEP8), programming errors (like “library imported but unused” and “Undefined name”) and to check cyclomatic complexity.
black is a code formatter. In essence it modifies the *.py files to be consistent with the defined rules. In this setup it would do the formatting in the background when commit command is executed.
auto-commit is a library that executes some code when the developer executes git commit command. For this change it means that auto-commit will trigger black and flake8 commands. If any of those commands return something else than 0 ('all good') the person will not be able to commit their changes (It is still possible to commit without style checking by adding --no-verify to the command. ).

After this PR is merged, each person who wants to commit to this repository would have to have precommit installed (possibly also flake8?). As you might notice, I have not changed any *.py files yet, this is because the checker and autoformatter works lazily - only on the files that will actually be committed. This means that the person who touches the file will be responsible to format it to a correct form if autoformatter is not decisive enough. I am happy to demonstrate this on one of the files if needed.

Example:
I have added one space in tests\tasks\test_multiplex_link_prediction.py file, added this file by git add *.py and executed git commit -m"Test" command.

image

Since there were failures, the code was not committed. Also, you can see that black modified the source code (files were modified by this hook). Now, it should be enough to manually fix flake8 errors (black ones were fixed automatically) and the commit should be good to go.
At the beginning, this might mean that there will be many false positives because as of now the code does not adhere to PEP8 very well, but because of laziness, we are guaranteed that the change is gradual. However, it is important to introduce consistency as early as possible - the more code there is to fix the worse the experience.

The flake8 and black configurations in this PR are purely my preference, I am happy to change those and/or have a philosophical discussion on whether this is a good configuration. In all projects that I am involved with I am using (almost) exactly this configuration.

@cenyk1230
Copy link
Member

Great suggestions! Thank you very much!

@cenyk1230
Copy link
Member

I find that the python version is specifically set as 3.6. Are there any ways to support more versions (e.g., 3.6, 3.7, and 3.8)?

@MaLiN2223
Copy link
Contributor Author

@cenyk1230 It is my understanding that setting to 3.6 will support everything 3.6+, however, some 3.7 and 3.8 features might not be checked for.
As far as I could find for black there is no difference between 3.6 and 3.8 because the syntax did not change. I think python did not introduce any big new syntax features since 3.6 (typing) so I think we should be good on that front.

@cenyk1230 cenyk1230 merged commit ffa8113 into THUDM:master Nov 28, 2020
@cenyk1230
Copy link
Member

@MaLiN2223 I'm afraid that the auto-commit is not automatically enabled. Could you please help to check whether something needs to be done?

@MaLiN2223
Copy link
Contributor Author

@cenyk1230 Of course, do you have any steps that I can reproduce the issue with?
Alternatively we can also add a git action to reject PR when it does not adhere to the guidelines.

@MaLiN2223 MaLiN2223 deleted the enforce_style branch December 16, 2020 13:08
@cenyk1230
Copy link
Member

@MaLiN2223 Sorry, it works. But I find that it needs all contributors to run pre-commit install in their machines to enable the pre-commit.

@MaLiN2223
Copy link
Contributor Author

@cenyk1230 that might be the case, if we want to mitigate that issue it would be okay to just autoformat with black and validate with autopep on github action but this makes the feedback loop longer (i.e. author would know that there are some style violations only after the pipeline finished).

@cenyk1230
Copy link
Member

@MaLiN2223 Thanks, I have formatted all the code in our repo in #93. Then we should require all the pull requests to follow the code format and style. Are there any quick style check solution that we can use in our travis testing? Maybe we should run flake8 in the before_script of travis.yml?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants