Skip to content

Commit

Permalink
Reformat and lint files with black and flake8 (#168)
Browse files Browse the repository at this point in the history
* Clean up of main plotting script

Format with black; fix many, many defects identified with pylint

* Change script names to conform with PEP8

Python prefers snake_case for script and module names
and this fixes pylint complaining about this

* Add flake8 and black targets

black target will reformat all Python scripts
flake8 will run linter on all Python scripts

* Reformat plotting script to improve linting

Passes flake8 (required) and most of pylint (recommended)

* Reformat to improve linting

Pass flake8 (required) and most of pylint (recommended)

* Add notes on Python formatting

black should be used for formatting
scripts must be error free with flake8

* Add flake8 run to actions

* Fix network test and trailing spaces

* Fix strange unicode chars in print string

* Fix name of Python2 http server module

* Refactor to avoid globals

pylint checks pushed an implementation where most code was inside the main() function,
however this caused a problem with unittest that will only run
tests in the global namespace,
but use of globals is rather nasty

This reimplementaion keeps the argument parsing and test case
generation in a main-ish function,
but returns the test case and then
invokes unittest.main()
which is a much nicer implementation

* Fix syntax for strategy

* Remove strategy matrix

Only need to run flake8 on one platform

* Whitelist codefactor/bandit urlopen()

The urlopen() call in these tests is to the http server that was setup by the
unittest itself, so we know that it's ok

* Fix flake8 error

This was an accident, but it proved that the flake8 GH action
works as we want!

Co-authored-by: Graeme A Stewart <graemes@pcphsft90.dyndns.cern.ch>
  • Loading branch information
graeme-a-stewart and Graeme A Stewart authored Sep 7, 2020
1 parent 049db24 commit 9d2decb
Show file tree
Hide file tree
Showing 30 changed files with 1,264 additions and 820 deletions.
7 changes: 7 additions & 0 deletions .flake8
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Minimal flake8 tweak to be compatible with black's line length
# of 88 characters
# https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length

[flake8]
max-line-length = 88
extend-ignore = E203
4 changes: 4 additions & 0 deletions .github/scripts/flake8.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#! /bin/sh
# Configure and run flake8 target
cmake /mnt
cmake --build . --target flake8
37 changes: 37 additions & 0 deletions .github/workflows/flake8.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Name of the workflow
name: flake8

# Controls when the action will run. Triggers the workflow on push or pull request
# events but only for the master and stable branches
on:
push:
branches: [ master, stable ]
pull_request:
branches: [ master, stable ]

# A workflow run is made up of one or more jobs that can run sequentially or in parallel
jobs:
# Main building and testing
build-test:
# The type of runner that the job will run on
runs-on: ubuntu-latest

# Steps represent a sequence of tasks that will be executed as part of the job
steps:
# Checks-out our repository under $GITHUB_WORKSPACE, so our job can access it
- uses: actions/checkout@v2

# Sets up useful environment variables
- name: Setup environment variables
run: |
echo "::set-env name=DIMAGE::hepsoftwarefoundation/u20-dev"
env:
PLATFORM: u20-dev

# Pulls the associated Docker image
- name: Docker pull
run: docker pull $DIMAGE

# Builds the code and runs the test
- name: Flake8 Lint
run: docker run -v $(pwd):/mnt $DIMAGE /mnt/.github/scripts/flake8.sh
3 changes: 2 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -94,5 +94,6 @@ add_subdirectory(package)
#--- create uninstall target ---------------------------------------------------
include(cmake/prmonUninstall.cmake)

#--- clang-format target -------------------------------------------------------
#--- code format targets -------------------------------------------------------
include(cmake/clang-format.cmake)
include(cmake/python-format.cmake)
21 changes: 21 additions & 0 deletions cmake/python-format.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Additional target to run python linters and formatters on python scripts
#
# Requires black/flake8 to be available in the environment


# Get all Python files
file(GLOB_RECURSE ALL_PYTHON_FILES *.py)

# Black is rather simple because there are no options...
add_custom_target(
black
COMMAND black
${ALL_PYTHON_FILES}
)

add_custom_target(
flake8
COMMAND flake8
--config=${CMAKE_CURRENT_SOURCE_DIR}/.flake8
${ALL_PYTHON_FILES}
)
8 changes: 7 additions & 1 deletion doc/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,16 @@ repository.

- the feature should be supported by an integration test
- the code should be commented as needed so that others can understand it
- code should be formatted using the *Google* style of `clang-format`
- C++ code should be formatted using the *Google* style of `clang-format`
- i.e. `clang-format --style=Google ...`
- There is a `clang-format` CMake target that will do this automatically so we
recommend that is run before making the pull request
- Python code should be formatted with `black` (following the Scikit-HEP
recommendation) and should also be clean with the `flake8` linter
- See the `.flake8` file for the configuration to be compatible with
`black`
- There are `black` and `flake8` build targets that will run the
formatter and linter respectively

We will review the code before accepting the pull request.

Expand Down
Loading

0 comments on commit 9d2decb

Please sign in to comment.