danionescu0 / docker-flask-mongodb-example

Uses docker compose with a python flask microservice and MongoDB instance to make a sample application
GNU General Public License v3.0
99 stars 41 forks source link

choosing a code formatter / style guide enforcer #65

Closed neelabalan closed 3 years ago

neelabalan commented 3 years ago

There are actually three python linting service autopep8, pylint and flake8

for flake8 we can have the following .flake8 in root folder

[flake8]
max-complexity = 10
select = C,E,F,W,B
ignore = E201, E501, E202

Here you can find what rules can be ignored (https://www.flake8rules.com)

and for black the precommit hook will look like this and this is the only configuration we will ever need and rest is taken care of. We can also use the--check and --diff argument to fail the commit if it doesn't meet black's required standards.

    -   repo: https://github.com/ambv/black
        rev: stable
        hooks:
        - id: black
          language_version: python3.8

autopep8 formats Python code to the PEP8 style. It fixes most of the formatting issues that are reported by pycodestyle

[pep8]
ignore = E226,E302,E41
max-line-length = 160

with yapf code formatting there is lot of options that can be set in config on how to format the code like


[style]
based_on_style = pep8
SPLIT_BEFORE_LOGICAL_OPERATOR = true
ARITHMETIC_PRECEDENCE_INDICATION = true
BLANK_LINE_BEFORE_NESTED_CLASS_OR_DEF = true
DEDENT_CLOSING_BRACKETS = true

Let me know with which one we can go ahead, I'll make those changes and raise a PR.

danionescu0 commented 3 years ago

Hmm i'll check it out, i'm searching for articles like this: https://www.slant.co/versus/12630/12632/~pylint_vs_flake8

Meanwhile what is your opinion what would you like to use in the project ?

neelabalan commented 3 years ago

My opinion is that using black can be good since we can get inplace formatting done based on standard style guides with minimal config.

This doc has all put together in one place https://books.agiliq.com/projects/essential-python-tools/en/latest/linters.html

danionescu0 commented 3 years ago

Ok let's go with black it seems simple. After the first run we might change some code too :)

Thanks for coming up with new ideas to improve the tutorial they are great!

neelabalan commented 3 years ago

Thank you! I will try to raise a PR today with pre-commit and code changes.

neelabalan commented 3 years ago

review before PR

There will be a pre-commit-config.yaml at root folder with following contents

repos:
  - repo: https://github.com/psf/black
    rev: 20.8b1 
    hooks:
      - id: black
        language_version: python3

pre-commit install to install the hooks

adding github actions

(If you think we can use this I'll add this too in the PR) .github/workflows/link.yml

name: Lint

on: [push, pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-python@v2
      - uses: psf/black@stable
        with:
          args: "python/ tests/ --check"

I've tested all of this in my private repo. I can add a CONTRIBUTIONS.md with detailed steps for future contributors if required.

danionescu0 commented 3 years ago

Ok add them both. for local development will we need to install some other dev tool for this ?

neelabalan commented 3 years ago

apart from pre-commit and black no dev tool is required.

danionescu0 commented 3 years ago

I've added a new file and it seems that is has some lint problems. Using this setup how am i able to see what i need to change ?

Attached is the output:

Run black --check --exclude stresstest-locusts/ . black --check --exclude stresstest-locusts/ . shell: /usr/bin/bash -e {0} env: pythonLocation: /opt/hostedtoolcache/Python/3.9.2/x64 LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.2/x64/lib would reformat /home/runner/work/docker-flask-mongodb-example/docker-flask-mongodb-example/python/tictactoe.py Oh no! 💥 💔 💥 1 file would be reformatted, 13 files would be left unchanged. Error: Process completed with exit code 1.

neelabalan commented 3 years ago

it shows would reformat /home/runner/work/docker-flask-mongodb-example/docker-flask-mongodb-example/python/tictactoe.py

danionescu0 commented 3 years ago

I saw that i should reformat tictactoe.py, but it doesn't show what to reformat

neelabalan commented 3 years ago

try adding --diff

neelabalan commented 3 years ago
~/temp >> black --check --diff .
would reformat sample.py
Oh no! 💥 💔 💥
1 file would be reformatted.
--- sample.py   2021-03-10 15:34:07.706973 +0000
+++ sample.py   2021-03-10 15:34:15.390279 +0000
@@ -1,4 +1,5 @@
-def testing(    ):
-    print('Hello'   )
+def testing():
+    print("Hello")
+

 print("Hello proper format")
neelabalan commented 3 years ago

to format the code in place you can either use pre-commit or black --exclude stresstest-locusts/ . without --check and --diff args

danionescu0 commented 3 years ago

I see. also could you update the test-build passing (https://github.com/neelabalan/docker-flask-mongodb-example/actions/workflows/test_build.yml) from the README.md with the one from this repo ?

danionescu0 commented 3 years ago

I think i'll install a local pre commit hook (https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/) to help be with checking the code before i commit it

neelabalan commented 3 years ago

I see. also could you update the test-build passing (https://github.com/neelabalan/docker-flask-mongodb-example/actions/workflows/test_build.yml) from the README.md with the one from this repo ?

I didn't understand. Do you want update the badge or documentation?

danionescu0 commented 3 years ago

i would like to update the badge to be linked with the current project, for example it should have been "build failed" before my latest commit (fix). Now it points to your fork so it does not help with the build status of the project

neelabalan commented 3 years ago

oops! sorry about that. I will make the change. The lint fail will not be updated there since the badge is referring to only the docker-compose build. I think a lint badge needs to be added seperately to show lint check status

danionescu0 commented 3 years ago

And if you like you can put some links to your repos in the readme i am not against that. But the readme should be correct we can't have a passing badge showing green when it's failed

neelabalan commented 3 years ago

And if you like you can put some links to your repos in the readme i am not against that. But the readme should be correct we can't have a passing badge showing green when it's failed

actually that was a mistake. I was testing the badge from my repo and before raising a PR I forgot to change that. I am not looking for adding my repo links here or in any other repo other than my own repos. I am here to learn tech by contributing.

danionescu0 commented 3 years ago

Ok, and i thanks you for contributing, i am also learining in an accelerated mode thanks to you :)

danionescu0 commented 3 years ago

It think we should close the issue, what do you think?

neelabalan commented 3 years ago

I agree