[WIP] Introduce code-quality checks to the repo (details TBD) #1099

devProdigy · 2020-12-09T18:51:19Z

What this PR offers:

check code-quality with flakehell (flake8 + plugins) and "black" on the CI before tests. In such way we'll eliminate waiting time to know that some space is missing.
- I know that there's a lot of legacy stuff here and no code-analysers (except black) was run previously, so this checks run only on changed code. Old code could be "unified" with time.
separate code-quality steps from tests
- here's CI job that shows how the flow looks like and how caching with pythonLocation speeds up the process. If I missed any downsides/risks of using pythonLocation instead of pip cache - please let me know. We should fix cache limit range.
- here's job that shows how CI will behave on quality-check error.

Motivation:

unifies code is easier to read;
code-checkers allow us not to spend code-review time on comments with formatting and styles discussions;
etc.

P.S. Previous PR was closed due to some un-reverted issues :) Here's lates comment from @flikka about this idea.
P.S.2. Feel free to add your opinion :)

* adjust README

devProdigy · 2020-12-09T18:55:46Z

.github/workflows/main.yml

+          path: ${{ env.pythonLocation }}
+          key: ${{ env.pythonLocation }}-${{ runner.os }}-${{ matrix.python-version }}-${{ hashFiles('**/*requirements.txt') }}


This speeds up the CI requirements install process for code-quality and tests jobs that run in parallel.

devProdigy · 2020-12-09T18:56:53Z

.github/workflows/main.yml

+      - uses: actions/cache@v2
+        with:
+          path: ~/.cache/pip


we can save some time on packages downloading. Don't know if this is worth it.

devProdigy · 2020-12-09T18:57:36Z

gordo/builder/build_model.py

@@ -145,7 +145,7 @@ def build(
                    model=model, machine=machine, output_dir=output_dir  # type: ignore
                )
                logger.info(f"Built model, and deposited at {self.cached_model_path}")
-                logger.info(f"Writing model-location to model registry")
+                logger.info("Writing model-location to model registry")


cause of this tests were failing.

codecov · 2020-12-09T18:58:18Z

Codecov Report

Merging #1099 (f6bc597) into master (6974cca) will not change coverage.
The diff coverage is 87.50%.

@@           Coverage Diff           @@
##           master    #1099   +/-   ##
=======================================
  Coverage   94.63%   94.63%           
=======================================
  Files          62       62           
  Lines        2965     2965           
=======================================
  Hits         2806     2806           
  Misses        159      159

Impacted Files	Coverage Δ
gordo/cli/custom_types.py	`89.47% <0.00%> (ø)`
gordo/builder/build_model.py	`98.39% <100.00%> (ø)`
gordo/cli/cli.py	`96.26% <100.00%> (ø)`
gordo/cli/client.py	`97.80% <100.00%> (ø)`
gordo/machine/validators.py	`86.88% <100.00%> (ø)`
gordo/reporters/mlflow.py	`97.85% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6974cca...66aa81e. Read the comment docs.

devProdigy · 2020-12-09T18:58:19Z

pyproject.toml

+flake8-bugbear = ["+*"]
+flake8-builtins = ["+*"]
+# - C812 exclude trailing comma for one line expression (black conflicts)
+flake8-commas   = ["+*", "-C812"]


we can add exclude that is logical from our perspective of view.

devProdigy · 2020-12-09T18:59:14Z

requirements/test_requirements.in

+flakehell~=0.7.0
+flake8-broken-line~=0.3
+flake8-bugbear~=20.11
+flake8-builtins~=1.5
+flake8-commas~=2.0
+flake8-comprehensions~=3.3
+darglint~=1.5
+flake8-debugger~=4.0
+flake8-docstrings~=1.5
+flake8-eradicate~=1.0
+flake8-isort~=4.0
+flake8-mutable~=1.2
+flake8-string-format~=0.3
+mccabe~=0.6
+pep8-naming~=0.11.1
+pycodestyle~=2.6
+pyflakes~=2.2


versions should be low and freezed cause of the massive conflicts in the requirements files.

devProdigy · 2020-12-09T18:59:53Z

setup.cfg

-# Black formatting
-testformatting   = pytest --addopts "tests/test_formatting.py"
-


black will be checked now on the CI step at first place, not to wait for the tests to end.

koropets · 2020-12-10T11:07:18Z

.github/workflows/main.yml

+
+      - name: Run code quality checks
+        run: |
+          FILES_TO_CHECK=$(git diff origin/master..origin/${{ steps.extract_branch.outputs.branch }} --name-only --diff-filter=ACM '*py')


Pull request could be created to another branch except for master. Also, it would not work with push into the branch which will not be merged into master.

Hm, good point. I'll see how to make origin/master dynamical.

About push - what do you mean? Diff will compare target branch with the current one and get the diff. Even if it push or pull_request.

koropets · 2020-12-10T11:10:03Z

.github/workflows/main.yml

+          pip install --upgrade pip-tools
+          pip install -r requirements/test_requirements.txt
+
+      - name: Run code quality checks


I think is better to reuse code from Makefile in this step:

run: | make ${{ matrix.code-check }}-check

There's different for CI approach and local. Locally we should check for any changes (not event committed), on the other hand on CI we should compare two remote branches.
So it'll be different diffs with different info needed to it.
So I split them not to make logic hard to understand and get lost among params.

.github/workflows/main.yml

Makefile

tests/gordo/server/test_base_view.py

devProdigy added 4 commits December 9, 2020 18:45

CI/Makefile/code adjustments

3ff5d24

add "bad code"

7a18384

change black's --diff on --check

3d828fe

* adjust README

fix "bad code" for testing

3a27db0

devProdigy mentioned this pull request Dec 9, 2020

[WIP] init commit for flakehell and black checks in the project #1088

Closed

devProdigy commented Dec 9, 2020

View reviewed changes

devProdigy requested a review from koropets December 9, 2020 19:00

koropets reviewed Dec 10, 2020

View reviewed changes

devProdigy added 7 commits December 10, 2020 15:52

PR adjustments

9015910

Merge branch 'master' into custom/add_black_and_flakehell_to_CI

5ee8476

measure amount of time to be used to check all files

e8b2a16

see if removing lines from output helps to save time

4407203

add _version to Gordo ignore

12ce9ad

test baseline

f6bc597

try aliases

66aa81e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Introduce code-quality checks to the repo (details TBD) #1099

[WIP] Introduce code-quality checks to the repo (details TBD) #1099

devProdigy commented Dec 9, 2020

devProdigy Dec 9, 2020

devProdigy Dec 9, 2020

devProdigy Dec 9, 2020

codecov bot commented Dec 9, 2020 •

edited

Loading

devProdigy Dec 9, 2020

devProdigy Dec 9, 2020

devProdigy Dec 9, 2020

koropets Dec 10, 2020

devProdigy Dec 10, 2020

koropets Dec 10, 2020

devProdigy Dec 10, 2020

		path: ${{ env.pythonLocation }}
		key: ${{ env.pythonLocation }}-${{ runner.os }}-${{ matrix.python-version }}-${{ hashFiles('*/requirements.txt') }}

		# Black formatting
		testformatting = pytest --addopts "tests/test_formatting.py"

[WIP] Introduce code-quality checks to the repo (details TBD) #1099

Are you sure you want to change the base?

[WIP] Introduce code-quality checks to the repo (details TBD) #1099

Conversation

devProdigy commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2020 •

edited

Loading