NCAS-CMS / cfunits

A Python interface to UNIDATA’s UDUNITS-2 library with CF extensions:
http://ncas-cms.github.io/cfunits
MIT License
11 stars 8 forks source link

Post-commit linting via new Actions workflow #23

Closed sadielbartholomew closed 3 years ago

sadielbartholomew commented 3 years ago

Pre-commit linting is now in-place after #21 and #22 but I will also add a new GH Actions workflow to do the equivalent linting on the codebase, effectively post-commit, since pre-commit hooks can be skipped meaning they don't ensure the new state of the codebase is passing the linting.

The workflow configuration is consistent with the pre-commit configuration of each linter applied, and it must remain so (I have commented as such in the new workflow file).

I've used a separate workflow to the existing one, run-test-suite.yml, since:

Note: I am adding sections in incrementally in the PR so I can check stages work and keep the trail-and-error contained.

codecov-io commented 3 years ago

Codecov Report

Merging #23 (406f5bc) into master (8e9b720) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #23   +/-   ##
=======================================
  Coverage   78.62%   78.62%           
=======================================
  Files           2        2           
  Lines         720      720           
=======================================
  Hits          566      566           
  Misses        154      154           
Flag Coverage Δ
unittests 78.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 8e9b720...406f5bc. Read the comment docs.

sadielbartholomew commented 3 years ago

Excellent, after a look-see at the jobs in question, both the black and the pre-commit jobs have linted the codebase as desired. Now to add in flake8 linting...

sadielbartholomew commented 3 years ago

Re-triggering via open-close (not elegant but it works!)...

sadielbartholomew commented 3 years ago

The new flake8 job passes but the logs don't provide any evidence it even ran which is not useful going forward. I might switch to use a different external action as there are numerous options for linting by flake8...

sadielbartholomew commented 3 years ago

The linting workflow didn't seem to run that time, that might indicate an issue. Trying again...

sadielbartholomew commented 3 years ago

The new workflow seems to not be running despite the test run workflow doing so and it being configured to run in the same cases, namely re-opening of a PR. I'll assume this is related to the fact the linting workflow is being added in this PR. If not I can tweak it via commits, to also test the checks run via push as well as via PR. So I'll merge this now.

sadielbartholomew commented 3 years ago

Postscript: I realised after merging that the pre-commit hook linting job runs the precise same checks as the pre-commits, therefore includes the black, flake8 and docs style linting, therefore that is the only step required! E.g, from the logs:

Run pre-commit/action@v2.0.0
install pre-commit
/opt/hostedtoolcache/Python/3.9.1/x64/bin/pre-commit run --show-diff-on-failure --color=always --all-files
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for https://github.com/ambv/black.
[INFO] Initializing environment for https://github.com/myint/docformatter.
[INFO] Initializing environment for https://github.com/pycqa/pydocstyle.
[INFO] Initializing environment for https://gitlab.com/pycqa/flake8.
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/ambv/black.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/myint/docformatter.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pycqa/pydocstyle.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://gitlab.com/pycqa/flake8.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Check python ast.........................................................Passed
Debug Statements (Python)................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
black....................................................................Passed
docformatter.............................................................Passed
pydocstyle...............................................................Passed
flake8...................................................................Passed
/bin/tar --use-compress-program zstd -T0 -cf cache.tzst -P -C /home/runner/work/cfunits/cfunits --files-from manifest.txt
Cache saved successfully

So in a subsequent commit I removed the sections where I ran black and flake8 separately, and the TODO to lint on pydocstyle and docformatter is automatically addressed. :tada:

sadielbartholomew commented 3 years ago

(Oops, my laptop clock is set on a different timezone due to some testing I was doing - hence the commit timestamps may not make sense in the context of these comments.)

davidhassell commented 3 years ago

Hi @sadielbartholomew, I'm trying to keep up! Does this mean that the GH action might change the code when you push you local commit to the remote?

sadielbartholomew commented 3 years ago

Hi @davidhassell,

Does this mean that the GH action might change the code when you push you local commit to the remote?

No, it should just check the code and report if there are failures. I wouldn't want anything to change our code between pushing commits and them actually being incorporated into the codebase, that sounds like a potential maintenance nightmare!

sadielbartholomew commented 3 years ago

@davidhassell, as a demo, I've just pushed the same new workflow into cfdm. You can see there are failures reported but nothing gets changed by Actions itself: https://github.com/NCAS-CMS/cfdm/runs/1981121843, we're just prompted to make those changes ourselves in future.

davidhassell commented 3 years ago

Got it (that's what I'd hoped!)