From 4598abe1ea39fee0110fd3299f18f52dbb2a85b2 Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Mon, 24 Jun 2024 10:54:24 +0200 Subject: [PATCH] Migrate files form the legacy doc-guidelines Those were migrated as is as they are still up to date. --- ANSIBLE.md | 103 ++++++ BASH.md | 371 +++++++++++++++++++++ DOCKER.md | 155 +++++++++ GITHUB_NAMES.md | 66 ++++ GIT_CHEAT_SHEET.md | 129 ++++++++ JAVASCRIPT.md | 334 +++++++++++++++++++ PYTHON.md | 782 +++++++++++++++++++++++++++++++++++++++++++++ PYTHON_PACKAGE.md | 207 ++++++++++++ README.md | 16 +- 9 files changed, 2162 insertions(+), 1 deletion(-) create mode 100644 ANSIBLE.md create mode 100644 BASH.md create mode 100644 DOCKER.md create mode 100644 GITHUB_NAMES.md create mode 100644 GIT_CHEAT_SHEET.md create mode 100644 JAVASCRIPT.md create mode 100644 PYTHON.md create mode 100644 PYTHON_PACKAGE.md diff --git a/ANSIBLE.md b/ANSIBLE.md new file mode 100644 index 0000000..a2e50e7 --- /dev/null +++ b/ANSIBLE.md @@ -0,0 +1,103 @@ +# Ansible guidelines + +- [Naming convention](#naming-convention) +- [Linting](#linting) +- [Formatting](#formatting) +- [Role vs Tasks](#role-vs-tasks) +- [Repository Structure](#repository-structure) + - [Collection](#collection) + +## Naming convention + +File, directory and variable names MUST be `snake_case`. Only use `yaml` files and use `*.yml` extension. + +## Linting + +Code should pass linting + +```bash +ansible-lint +``` + +## Formatting + +Code should be formatted as follow: + +- Use space instead of tab +- Use 2 spaces indentation +- YAML files should always start with `---` and end with a blank line + + ```yaml + --- + # This is an example of correct yaml file formatted + my_yaml: + content: hello world + + ``` + +You should always use the formatting tool of ansible-lint + +```bash +ansible-lint --write all +``` + +## Role vs Tasks + +For reusable code prefer Roles over Tasks. If some code could reused than encapsulate it in a role, +otherwise simply add it to your playbook. + +## Repository Structure + +Repository should be prefixed by; `infra-ansible-` + +And they should follow this structure + +```text +├── files +│ └── some-file.txt +├── group_vars +│ └── group_name.yml +├── hosts_vars +│ └── host_name.yml +├── roles +│ └── my_role +│ ├── handlers +│ │ └── main.yml +│ ├── defaults +│ │ └── main.yml +│ ├── files +│ │ └── some-file.txt +│ ├── tasks +│ │ └── main.yml +│ ├── vars +│ │ └── main.yml +│ └── README.md +├── vars +│ └── main.yml +├── playbook_1.yml +├── playbook_2.yml +└── inventory.yml +``` + +### Collection + +For collection we follow the ansible standard folder structure + +```text +├── roles +│ └── bgdi_toolchains +│ ├── handlers +│ ├── defaults +│ ├── files +│ ├── tasks +│ │ └── main.yml +│ ├── vars +│ └── README.md +├── playbooks +| ├── files +| ├── vars +| ├── templates +│ └── my-playbook.yml +├── galaxy.yml +└── README.md +``` diff --git a/BASH.md b/BASH.md new file mode 100644 index 0000000..6b4391b --- /dev/null +++ b/BASH.md @@ -0,0 +1,371 @@ +# Bash coding guidelines + +There are a number of styleguides out there. Check what [Google Shell Style Guide](https://google.github.io/styleguide/shellguide.html) proposes, we largely follow this document. A few important things are reproduced here. + +- [1. When to use BASH scripts](#1-when-to-use-bash-scripts) +- [2. Best Practices](#2-best-practices) + - [basic shell script structure](#basic-shell-script-structure) + - [1. shebang header](#1-shebang-header) + - [2. function sections](#2-function-sections) + - [3. main part of the script](#3-main-part-of-the-script) + - [Variable expansion](#variable-expansion) + - [Strict mode](#strict-mode) + - [This option also has some downsides](#this-option-also-has-some-downsides) + - [Trap](#trap) + - [Multiline comments](#multiline-comments) + - [Bash functions command groups vs sub shell](#bash-functions-command-groups-vs-sub-shell) +- [3. Linting / shellcheck](#3-linting--shellcheck) + - [shellcheck options](#shellcheck-options) +- [4. Unit Tests / shellspec](#4-unit-tests--shellspec) + - [Installation of shellspec](#installation-of-shellspec) + - [Add new unit tests](#add-new-unit-tests) + - [Execute unit tests](#execute-unit-tests) + - [mocking](#mocking) + - [overwrite functions / variables](#overwrite-functions--variables) + - [set up / tear down functions for more complex input (folders and files, json objects, etc.)](#set-up--tear-down-functions-for-more-complex-input-folders-and-files-json-objects-etc) + - [To be noted](#to-be-noted) + +With this [handy tool](https://explainshell.com/) you can analyze and explain bash command-lines. + +**The foremost goal is that reading and understanding your bash script is easy for someone else (or yourself in a few months time or after a memory reset)** + +**Another important goal is to follow the ``Parting words`` section in this document: https://google.github.io/styleguide/cppguide.html#Parting_Words** + +## 1. When to use BASH scripts + +Bash scripts should only be used for: + +- helper scripts +- wrapper scripts (p.e data integration ogr / psql wrapper, db deploy scripts, ...) +- small utilities + +See also this overview [here](https://opensource.com/article/19/4/bash-vs-python) for a comparison of bash and python pros and cons. + +If you decide to write a bash script, make sure to follow the descriptions and best practices below. + +## 2. Best Practices + +### basic shell script structure + +A basic Bash script has three sections. + +#### 1. shebang header + +The first line of the bash script should be the shebang header, ``#!/bin/bash`` in our case for Bourne-again Shell. + +#### 2. function sections + +In this section you should + +- define functions, variables and constants +- include other scripts + +At the end of this section you should add this line: + +```bash +[ "$0" = "${BASH_SOURCE[*]}" ] || return 0 +``` + +Everything under this line wont be executed when you source the file with + +```bash +$ source script.s +# or +$ . script.sh +``` + +#### 3. main part of the script + +This can be a single bash statement, several function calls or thousands of lines of code. + +### Variable expansion + +Variable or parameter expansion is a really powerful feature in bash. +For example, you can react to different states of variables and assign default values: + +```bash + +----------------------+------------+-----------------------+-----------------------+ + | if VARIABLE is: | set | empty | unset | + +----------------------+------------+-----------------------+-----------------------+ + - | ${VARIABLE-default} | $VARIABLE | "" | "default" | + = | ${VARIABLE=default} | $VARIABLE | "" | $(VARIABLE="default") | + ? | ${VARIABLE?default} | $VARIABLE | "" | exit 127 | + + | ${VARIABLE+default} | "default" | "default" | "" | + +----------------------+------------+-----------------------+-----------------------+ +:- | ${VARIABLE:-default} | $VARIABLE | "default" | "default" | +:= | ${VARIABLE:=default} | $VARIABLE | $(VARIABLE="default") | $(VARIABLE="default") | +:? | ${VARIABLE:?default} | $VARIABLE | exit 127 | exit 127 | +:+ | ${VARIABLE:+default} | "default" | "" | "" | + +----------------------+------------+-----------------------+-----------------------+ +``` + +[Here](https://wiki-dev.bash-hackers.org/syntax/pe#indirection) you will find more examples of what else is possible with variable expansion. + +### Strict mode + +In order to make our scripts more solid, we always set the following options on top of the script: + +```bash +set -euo pipefail +``` + +The ``set -e`` option instructs bash to immediately exit if any command [1] has a non-zero exit status. +The ``set -u`` option affects variables. When set, a reference to any variable you haven't previously defined - with the exceptions of ``$*`` and ``$@`` - is an error, and causes the program to immediately exit. + +#### This option also has some downsides + +if you try to **test if a variable exists**. In that case it is recommended to use variable expansion: + +```bash +# For those that are looking to check for unset or empty when in a script with set -u: +if [ -z "${var-}" ]; then + echo "Must provide var environment variable. Exiting...." + exit 1 +fi +``` + +if you try to count the number of elements in an optional array (which could also be a variable of type text), you would have to use something like this: + +```bash +if declare -p my_array | grep -q '^declare \-a'; then + length=${#my_array[@]} +else + length=0 +fi +``` + +The ``set -o pipefail`` option will make the bash shell look at the exit code of all the commands in a pipeline. Without this option, the shell option ``-e`` only reacts on the exit status of the last command of the pipeline. + +### Trap + +Trap allows you to catch system signals and some user signals and execute code when they occur. This is useful when you want to do some clean-up, remove lock files or unblock external resources etc. when the script exits abnormally (or normally). +You will find a lot of examples in our [data processing scripts](https://github.com/geoadmin/bgdi-scripts/search?q=trap&unscoped_q=trap). + +### Multiline comments + +multiline comments are best done this way: + +```bash +#!/bin/bash +: ' +This is a +very neat comment +in bash +' +``` + +### Bash functions command groups vs sub shell + +There are two ways to to [group commands](https://www.gnu.org/software/bash/manual/html_node/Command-Grouping.html) and create functions in bash: + +```bash +function_1() { +# function defined with command group + +# commands in a command group are executed in the current shell +# they have read-write access to all the variables of the current shell +# exit will stop the current shell and the script itself +} + +# or +function_2() ( +# function defined with sub shell + +# the scope of all variables defined here is limited to the subshell +# you cant change the environment variables of the calling shell +# exit will kill stop the subshell only +) +``` + +try read and understand first, when you're editing an existing script: https://google.github.io/styleguide/cppguide.html#Parting_Words + +## 3. Linting / shellcheck + +Each script should be checked with [shellcheck](https://github.com/koalaman/shellcheck). + +ShellCheck is a static analysis and linting tool for sh/bash scripts, the goals of ShellCheck are + +- To point out and clarify typical beginner's syntax issues that cause a shell to give cryptic error messages. +- To point out and clarify typical intermediate level semantic problems that cause a shell to behave strangely and counter-intuitively. +- To point out subtle caveats, corner cases and pitfalls that may cause an advanced user's otherwise working script to fail under future circumstances. + +There are different ways to use shellcheck. You can use shellcheck from the terminal or you can add it as plugin to vi, nvim, tmux, etc. + +### shellcheck options + +You can configure shellcheck with [directives](https://github.com/koalaman/shellcheck/wiki/Directive). +If you want to exclude some shellcheck codes from the report you can define the excluded codes either as command line argument: + +```text + -e CODE1[,CODE2...], --exclude=CODE1[,CODE2...] + Explicitly exclude the specified codes from the report. Subsequent -e options are + cumulative, but all the codes can be specified at once, comma-separated as a single + argument. +``` + +or as a comment in the first line after the shebang of the file: + +```bash +#!/bin/bash +# shellcheck disable=CODE1,CODE2,... +``` + +Starting with version ``0.7.0`` of shellcheck you can set most of the options in a ``.shellcheckrc`` rc file in the project’s root directory (or your home directory). This allows you to easily apply the same options to all scripts on a per-project/directory basis. + +We are using the following options: + +- executables + + ```bash + # SC2029: https://github.com/koalaman/shellcheck/wiki/SC2029 you can disable this check if you want your string to be expanded client-side + # SC2087: https://github.com/koalaman/shellcheck/wiki/SC2087 you can disable this check if you want the here document to be expanded on client-side + disable=SC2029,SC2087 + ``` + +- libraries, includes + + ```bash + # SC2034: https://github.com/koalaman/shellcheck/wiki/SC2034 you can disable this check if you declare variables which are used in the sourcing scripts + disable=SC2034 + ``` + +## 4. Unit Tests / shellspec + +Bash unit tests can be configured and executed with the [shellspec framework](https://shellspec.info/). +The shellspec framework has been chosen because it is: + +- powerful +- easy to configure +- has different reporting formats (p.e TAP, JUnit) +- can be used in CI +- based on pure shell +- supports different shells +- can be installed anywhere ``curl | sh`` + +documentation : [md files on github](https://github.com/shellspec/shellspec/blob/master/docs) + +Currently there are bash unit tests for the following projects: + +- [dataprocessing crontab scripts / includes](https://github.com/geoadmin/bgdi-scripts/blob/master/geodata_cron_skripte/bgdipg01t/spec/bgdipg01t_spec.sh) +- [db deploy script](https://github.com/geoadmin/deploy/blob/master/db_master_deploy/spec/db_master_deploy_spec.sh) + +### Installation of shellspec + +shellcheck can be installed anywhere with: + +```bash +curl -fsSL https://git.io/shellspec | sh -s <> -p <> -y +``` + +If you install it to a custom directory, make sure to add the directory to the PATH variable. +Please check the documentation for further information about installation, configuration, etc. + +You can also run it with docker using the image described here https://github.com/geoadmin/docker-shellspec + +In our projects we have a Makefile target for the installation and execution of the unit tests. + +```bash +# installation +make all +# execution +make bash_unit_tests +``` + +### Add new unit tests + +If you want to create unit tests, you have execute the following command in the folder with the shell scripts first: + +```bash +shellspec --init + +# this will create the following files and folders: +# create .shellspec -> configuration of default options +# create spec/spec_helper.sh +# create spec/geodata_cron_skripte_spec.sh -> <>_spec.sh +``` + +Then you can add the unit tests to the file ``spec/<>_spec.sh``. +Once the tests are ready, do not forget to add them to github. + +### Execute unit tests + +Make sure to go down to the parent folder of the unit test folder ``spec`` before you run the tests: + +```bash +cd geodata_cron_skripte/bgdipg01t/ +# run the tests +shellspec -s bash spec/bgdipg01t_spec.sh +# or choose an example by pattern +shellspec -s bash spec/bgdipg01t_spec.sh --example '*data_env*' +``` + +### mocking + +You can simulate a **stable input** for your unit tests with **mocking**. Mocking is creating objects or function input that simulate the behavior of real objects. There are several ways to create your stable unit test environment. + +#### overwrite functions / variables + +You can find a simple example of mocking [here](https://shellspec.info/#easy-to-mock--stub). + +#### set up / tear down functions for more complex input (folders and files, json objects, etc.) + +you can initialize your test data / input / objects with a function which is executed at the beginning of the unit tests and which is creating the test data / input / objects in a reproducible / isolated way. + +```bash +Describe 'some unit tests which need test folder with test data as input' +Include ./file_with_functions_to_test.sh +mock_set_up() { +# create test data for the following unit tests +# we need a folder mock_data with one file +# mock_data/ +# └── input.txt + +rm -rf mock_data &> /dev/null || : +cat << EOF > "mock_data/input.txt" +Title: unit test +Date and time: 2017-08-25T11:00+02:00 +Abstract: Bondo Rockfall 23.08.2017, quick orthophoto from 25.08.2017 taken with the aerial sensor ADS100, GSD 14cm +Link to viewer: https://s.geo.admin.ch/86166fc2f1 +Link to event: +Image type: RGBN +EOF +} +# Set Up / Initialize Test data +mock_set_up + +Sample 'function1_success' +... +End + +Sample 'function1_fail' +# if you first test has changed the test data, re-run the function +mock_set_up +... +End +... + +# Tear Down +# In the end clear test data +rm -rf mock_data &> /dev/null || : +End +``` + +You will find more examples in the unit tests folder of the cron scripts: ``geodata_cron_skripte/bgdipg01t/spec/bgdipg01t_spec.sh`` + +### To be noted + +- If you want to write unit tests on the functions of an existing script you have to include the script in the shellspec Example / unit tests with ``Include myscript.sh`` +Before you include an existing script, make sure that nothing is executed when you Include the file. Just add the following line before the first execution of a command in the script: + +```bash +# do not execute anything if file is sourced for unit tests +[ "$0" = "$BASH_SOURCE" ] || return 0 +``` + +- There are different ways to run commands or function. Please read the [documentation](https://github.com/shellspec/shellspec/blob/7f6dd12d356d5259bbcf83e7ee7fd6b25a885b37/docs/references.md#example) about that. +In most cases you can use either + - ``When call`` or + - ``When run`` + +With ``When call`` you have access to the variables which are defined inside the function +p.e. ``The variable RESULT_MAIL_CONTENT should include ">f+++++++++ event1/data.tif"``. diff --git a/DOCKER.md b/DOCKER.md new file mode 100644 index 0000000..a1ca990 --- /dev/null +++ b/DOCKER.md @@ -0,0 +1,155 @@ +# Docker + +- [1. Base images](#1-base-images) +- [2 Best practices](#2-best-practices) + - [Do not run as “root”](#do-not-run-as-root) + - [Ordering of layers](#ordering-of-layers) + - [Limit number of layers](#limit-number-of-layers) + - [Sort multi-line arguments](#sort-multi-line-arguments) + - [Use multistage builds](#use-multistage-builds) +- [Recipies](#recipies) + - [Create unprivileged user with host user id](#create-unprivileged-user-with-host-user-id) + +## 1. Base images + +Whenever possible, official docker images based on `buster-slim` should be used (e.g. `python:3.7-buster-slim`, `nginx:1.19`, ...). Customized base and application images should be based on `buster-slim`. + +## 2 Best practices + +There’s plenty of sources on this topic, e.g. + +- [https://docs.docker.com/develop/develop-images/dockerfile_best-practices/](https://docs.docker.com/develop/develop-images/dockerfile_best-practices/) +- [https://snyk.io/blog/10-docker-image-security-best-practices/](https://snyk.io/blog/10-docker-image-security-best-practices/) +- [https://cloud.google.com/solutions/best-practices-for-building-containers](https://cloud.google.com/solutions/best-practices-for-building-containers) +And specifically for python +- [https://pythonspeed.com/docker/](https://pythonspeed.com/docker/) +- [https://pythonspeed.com/articles/dockerizing-python-is-hard/](https://pythonspeed.com/articles/dockerizing-python-is-hard/) + +The most important ones are reproduced here for reference: + +- [1. Base images](#1-base-images) +- [2 Best practices](#2-best-practices) + - [Do not run as “root”](#do-not-run-as-root) + - [Ordering of layers](#ordering-of-layers) + - [Limit number of layers](#limit-number-of-layers) + - [Sort multi-line arguments](#sort-multi-line-arguments) + - [Use multistage builds](#use-multistage-builds) +- [Recipies](#recipies) + - [Create unprivileged user with host user id](#create-unprivileged-user-with-host-user-id) + +### Do not run as “root” + +This is a good practice in general and required by our Kubernetes cluster setup. The user can be changed with the `USER` instruction and requires that the user exists and has the correct permissions to access the relevant files. A minimal example could look like this + +```Dockerfile +FROM python +RUN mkdir /app +RUN groupadd -r web && useradd -r -s /bin/false -g web web +WORKDIR /app +COPY index.html /app +RUN chown -R web:web /app +USER web +EXPOSE 8080 +CMD ["python", "-m", "http.server", "8080"] +``` + +### Ordering of layers + +The build process can considerably be speed up if the layers are ordered in good way, starting with layers that change least frequently (typically the base image mentioned with `FROM`) down to the layer that changes most frequently (e.g. the `COPY` command copying the source files in the container). If the image is constructed like this, a change in the code just requires one layer to be rebuilt. + +```Dockerfile + +FROM debian-buster > the base layer + +RUN apt-get update && \ \ + apt-get install -y \ | + gosu \ > the 'system' layer + git \ | + vim / + +COPY index.html /app > the 'code' layer +``` + +### Limit number of layers + +Each `RUN`, `CMD`, `FROM`, `COPY`, `ADD`... instruction creates a new layer. The number of layers can be e.g. be reduced by chaining (AND-ing) shell commands: `RUN apt-get update && apt-get install -y git` + +### Sort multi-line arguments + +Whenever possible, ease later changes by sorting multi-line arguments alphanumerically. This helps to avoid duplication of packages and make the list much easier to update. This also makes PRs a lot easier to read and review. Adding a space before a backslash (`\`) helps as well. + +Here’s an example from the buildpack-deps image: + +```Dockerfile +RUN apt-get update && apt-get install -y \ + bzr \ + cvs \ + git \ + mercurial \ + subversion +``` + +### Use multistage builds + +[Multi-stage builds](https://docs.docker.com/develop/develop-images/multistage-build/) allow you to drastically reduce the size of your final image, without struggling to reduce the number of intermediate layers and files. + +Because an image is built during the final stage of the build process, you can minimize image layers by leveraging build cache. + +For example, if your build contains several layers, you can order them from the less frequently changed (to ensure the build cache is reusable) to the more frequently changed: + +- Install tools you need to build your application +- Install or update library dependencies +- Generate your application + +A Dockerfile for a Go application could look like: + +```Dockerfile +FROM golang:1.11-alpine AS build + +# Install tools required for project +# Run `docker build --no-cache .` to update dependencies +RUN apk add --no-cache git +RUN go get github.com/golang/dep/cmd/dep + +# List project dependencies with Gopkg.toml and Gopkg.lock +# These layers are only re-built when Gopkg files are updated +COPY Gopkg.lock Gopkg.toml /go/src/project/ +WORKDIR /go/src/project/ +# Install library dependencies +RUN dep ensure -vendor-only + +# Copy the entire project and build it +# This layer is rebuilt when a file changes in the project directory +COPY . /go/src/project/ +RUN go build -o /bin/project + +# This results in a single layer image +FROM scratch +COPY --from=build /bin/project /bin/project +ENTRYPOINT ["/bin/project"] +CMD ["--help"] +``` + +## Recipies + +### Create unprivileged user with host user id + +Sometimes it's useful or necessary to use an unprivileged user in the container that has the same uid as the host user executing the container. This is e.g. the case if you mount a volume and have the container create files. This can be achieved with the following snippet: + +```Dockerfile +... +# Create unprivileged user +# # gosu is used to create an unprivileged user +# at `docker run`-time with the same uid as the host user. Thus, the mounted +# host volume has the correct uid:guid permissions. For details: +# https://denibertovic.com/posts/handling-permissions-with-docker-volumes/ +# Installation of gosu: +# https://github.com/tianon/gosu/blob/master/INSTALL.md +RUN set -eux; \ + apt-get update; \ + apt-get install -y \ + gosu \ + rm -rf /var/lib/apt/lists/*; \ +# verify that the binary works + gosu nobody true +``` diff --git a/GITHUB_NAMES.md b/GITHUB_NAMES.md new file mode 100644 index 0000000..7a27d64 --- /dev/null +++ b/GITHUB_NAMES.md @@ -0,0 +1,66 @@ +# Naming convention for repositories + +All repository names shall be lower case only. +All repository names shall have a prefix, followed by a dash. +All repository names shall use a dash for separation. +Forked repositories from 3rd party sources are exceptions. + +## Prefixes + +### action- + +Github custom action (see [About custom actions](https://docs.github.com/en/actions/creating-actions/about-custom-actions)). + +### app- + +small windows applications + +### addin- + +Addins or plugins + +### cw- + +Cartoweb applications + +### doc- + +Documentation and Specifications + +### lib- + +Libraries + +### mf- + +Mapfish Framework applications + +### service- + +Services + +### tool- + +Tools and Scripts + +### web- + +Web applications and pages + +### wms- + +WMS Services + +### infra- + +projects related with infrastructure + +In addition to the prefix the following pattern should be followed: `infra-[technology/type]-[project/aws-account]` (eg. `infra-terraform-bgdi-dev`) + +### suite- + +a suite of applications and services + +### config- + +config files for service diff --git a/GIT_CHEAT_SHEET.md b/GIT_CHEAT_SHEET.md new file mode 100644 index 0000000..8e7d061 --- /dev/null +++ b/GIT_CHEAT_SHEET.md @@ -0,0 +1,129 @@ +# GIT Cheat Sheet + +- [Status and Config](#status-and-config) +- [Update repository from remote](#update-repository-from-remote) +- [Branching](#branching) +- [Git History](#git-history) +- [Diff and merge tool](#diff-and-merge-tool) +- [Commiting](#commiting) +- [Merge, Rebase and Merge Tool](#merge-rebase-and-merge-tool) + - [Mergetool LOCAL BASE REMOTE](#mergetool-local-base-remote) +- [Reverting changes](#reverting-changes) +- [Clean up](#clean-up) +- [Push change on remote](#push-change-on-remote) + +## Status and Config + +| Command | Description | +|---------|-------------| +| `git status` | Shows the status of the current repository | +| `git --no-pager config --global -l` or `git config --global -l` | List the global git configuration | +| `git --no-pager config -l` or `git config -l` | List the current repository configuration | + +See also [GIT Configuration](./GIT_FLOW.md#git-configuration). + +## Update repository from remote + +| Command | Description | +|---------|-------------| +| `git fetch` | Fetch all remotes | +| `git fetch ` | Fetch given remote | +| `git fetch -p` | Fetch all remotes (`-p` removes remotes branch that have been deleted) | +| `git pull` | Fetch remote and integrate changes into local branch. Pull does either a `merge` or `rebase` based on the configuration, at swisstopo we should use `git pull --rebase`, so make sure you have `pull.rebase=true` in your git config (see [GIT Configuration](./GIT_FLOW.md#git-configuration)) | +| `git fetch origin develop:develop` | Shortcut for `git checkout develop; git pull; git checkout `. NOTE: the branch develop can be changed with another branch name | + +## Branching + +| Command | Description | +|---------|-------------| +| `git branch` | List all local branches | +| `git branch -a` | List all local and remote branches | +| `git checkout -b ` | Create and checkout a new branch based on the current branch | +| `git branch ` | Create a new branch at the current position | +| `git branch -d ` | Delete branch | + +## Git History + +| Command | Description | +|---------|-------------| +| `gitk --all &` | Open the whole git history in a graphical view | +| `git log -n` | Display the n last commits message | +| `git log --graph --all` | Show the whole git history with commit message in the command line | +| `git log --graph --all --online` | Show the whole git history without commit message in the command line | + +The following command are very useful when working on a remote server with shell only, but it highly recommended to set them as alias + +| Command | Description | +|---------|-------------| +| `git log --graph --abbrev-commit --decorate --format=format:'%C(bold blue)%h%C(reset) - %C(bold green)(%ar)%C(reset) %C(white)%s%C(reset) %C(dim white)- %an%C(reset)%C(bold yellow)%d%C(reset)' --all` | Print a nice consice git history on the commande line including relative date and commit author | +| `git log --graph --abbrev-commit --decorate --format=format:'%C(bold blue)%h%C(reset) - %C(bold cyan)%as%C(reset) %C(bold yellow)%d%C(reset) %C(white)%s%C(reset) %C(dim white)- %an%C(reset)' --all` | Print a nice consice git history on the command line including author date and commit author | + +## Diff and merge tool + +| Command | Description | +|---------|-------------| +| `git diff` | Print a unified diff of the local changes | +| `git diff -- ':!file1.txt'` | Print a unified diff excluding the file `file1.txt` from the output | +| `git difftool` | Open the local changes with an external difftool (e.g. Beyond Compare) | +| `git mergetool` | Open conflict with external 3 way difftool for conflict resolution (e.g. Beyond Compare), see also below [Merge and Rebase](./GIT_FLOW.md#merge-and-rebase) | + +## Commiting + +| Command | Description | +|---------|-------------| +| `git add .` | Add all files into staging area | +| `git add ` | Add file(s) into staging area, file can contain wild card | +| `git add -p ` | Add only parts of a file into staging area | +| `git commit --amend` | Change last commit message and/or add staging area to last commit | +| `git commit -m ""` | Commit the staging area with the given message | +| `git commit` | Commit the staging area, the default editor is then popup for the commit message | + +## Merge, Rebase and Merge Tool + +| Command | Description | +|---------|-------------| +| `git rebase develop` | Rebase the current branch on top of develop | +| `git rebase -i develop` | Interactively rebase all commits up to the branch develop | +| `git rebase -i HEAD~3`| Interactively rebase the last 3 commits | +| `git reset --hard ORIG_HEAD` | Revert the last rebase. :warning: Create a temporary branch first for safety. | +| `git merge ` | Merge the given branch into the current one | +| `git mergetool` | Open conflict with external 3 way difftool for conflict resolution (e.g. Beyond Compare) | + +### Mergetool LOCAL BASE REMOTE + +`git rebase` + +- `LOCAL` = the base you're rebasing onto +- `REMOTE` = the commits you're moving up on top + +`git merge` + +- `LOCAL` = the original branch you're merging into +- `REMOTE` = the other branch whose commits you're merging in + +In other words, `LOCAL` is always the original, and `REMOTE` is always the guy whose commits weren't there before, because they're being merged in or rebased on top + +## Reverting changes + +| Command | Description | +|---------|-------------| +| `git checkout -- .` | Revert all local changes. WARNING CHANGES ARE THEN LOST ! | +| `git reset HEAD~` | Revert last commit to the working area | +| `git reset --soft HEAD~` | Revert last commit into the staging area | +| `git reset --hard ` | Revert all locals, staging and commits up to the given branch or commit or remote. ***WARNING CHANGES ARE THEN LOST !*** | + +## Clean up + +| Command | Description | +|---------|-------------| +| `git clean -xdfn` | Dry run clean the whole repository and print what would be deleted | +| `git clean -xdf` | Clean the whole repository, and bring the repo back to a fresh clone. ***WARNING THIS CANNOT BE REVERTED AND DATA MIGHT BE LOST ! CONSIDER USING `-xdfn` FIRST.*** | + +## Push change on remote + +| Command | Description | +|---------|-------------| +| `git push origin HEAD` | Push the current branch to the remote origin | +| `git push origin ` | Push the branch to the remote origin | +| `git push origin -f` | Force push the branch to the remote origin. ***WARNING DO THIS ONLY ON PERSONAL BRANCH*** | +| `git push` | By default, it push the current branch to the remote origin. ***WARNING: Use with caution, the default behavior might change due to global configuration or branch configuration, consider using `git push origin ` instead.*** | diff --git a/JAVASCRIPT.md b/JAVASCRIPT.md new file mode 100644 index 0000000..282499d --- /dev/null +++ b/JAVASCRIPT.md @@ -0,0 +1,334 @@ +# Javascript coding guidelines + +There are a number of style guides out there. Here a few important things that we should follow: + +- [1. Linting / Auto-formatting](#1-linting--auto-formatting) + - [Formatting with Prettier](#formatting-with-prettier) + - [Linting with Eslint](#linting-with-eslint) + - [Ignore linting warning/refactoring/convention](#ignore-linting-warningrefactoringconvention) +- [2. Naming conventions](#2-naming-conventions) +- [3. Comments and JSDoc](#3-comments-and-jsdoc) +- [4. Single statement](#4-single-statement) +- [5. Vue 3 Best Practice](#5-vue-3-best-practice) + - [Styling](#styling) + - [Component data](#component-data) + - [Component method](#component-method) + - [Definition](#definition) + - [Method called from template](#method-called-from-template) +- [Debugging on Mobile Phone](#debugging-on-mobile-phone) + +**The foremost goal is that reading and understanding your javascript code is easy for someone else (or yourself in a few months time).** + +## 1. Linting / Auto-formatting + +### Formatting with Prettier + +For our javascript projects we are using [Prettier](https://prettier.io/) to automatically format your code. We highly recommend to configure you IDE to format your code using Prettier on Save. Here below is a Prettier configuration example + +```yaml +printWidth: 100 +singleQuote: true +semi: false +trailingComma: es5 +tabWidth: 4 +jsxSingleQuote: false +jsdocParser: true +jsdocDescriptionWithDot: true +jsdocVerticalAlignment: true +overrides: + - files: '*.md' + options: + tabWidth: 2 + proseWrap: 'always' + +``` + +### Linting with Eslint + +Although formatting is good, it doesn't check for syntax errors or bad code practice. +Therefore we also use a linter; [Eslint](https://eslint.org/) + +Usually we should have such ESlint configuration: + +```json +{ + "env": { + "es2021": true, + "node": true + }, + "plugins": ["prettier"], + "extends": [ + "eslint:recommended", + "plugin:vue/essential", + "prettier", + "plugin:vue/recommended", + "plugin:cypress/recommended", + "plugin:prettier-vue/recommended", + ], + "parserOptions": { + "ecmaVersion": 12, + "parser": "babel-eslint", + }, + "rules": { + "camelcase": ["error", { "properties": "never" }], + "curly": "error", + "max-len": ["warn", { "code": 100 }], + "no-unused-vars": "warn" + } +} +``` + +#### Ignore linting warning/refactoring/convention + +There might be good reason to disable locally some linting messages. When doing this, the reason why we disable a rule +should be documented next to the disable pragma. + +For more detail in ignoring eslint issues see +[ESLint Disabling Rules](https://eslint.org/docs/user-guide/configuring/rules#disabling-rules) + +## 2. Naming conventions + +Javascript code must follow these naming conventions: + +- file name: camelCase +- constant: UPPER_CASE +- variable: camelCase +- function/method: camelCase +- argument: camelCase +- class: PascalCase + +## 3. Comments and JSDoc + +Code is documented using [JSDoc](https://jsdoc.app/). Make sure to document every public class and functions. + +## 4. Single statement + +ALWAYS enclosed with curly brace single if/else/while/do while/for single statements + +:x: **Incorrect** + +```javascript +if (test) return + +if (test) + doSomething() + +while(1) + doSomething() + +for (let i=0; i < test; i++) doSomething() +``` + +:white_check_mark: **Correct** + +```javascript +if (test) { + return +} + +if (test) { + doSomething() +} + +while(1) { + doSomething() +} + +for (let i=0; i < test; i++) { + doSomething() +} +``` + +## 5. Vue 3 Best Practice + +See first [Vue 3 Best Practice](https://vuejs.org/v2/style-guide/) + +### Styling + +- Always use directive shorthands; `:` for `v-bind`, `@` for `v-on`, `#` for `v-slot` +- Always use Single-file component whenever possible +- Single-file component must always have the following element order + + ```html + + + + ``` + +- Use indentation within `` tag but not on `` and `` tags + + ```html + tag omit the first indentation level' + const isScript = true + + if (isScript) { + console.log(myString) + } + + + + + + ``` + +- Avoid using element selector with `scoped` (see [Vue 3 Best Practice - Element selectors with `scoped`](https://vuejs.org/v2/style-guide/#Element-selectors-with-scoped-use-with-caution)), but use `scoped` class or id selector instead. + + ```html + + + + + ``` + + ```html + + + + + ``` + +- Always add the `*.vue` extension in import of Vue single file component. This is required by some IDE (Vscode/Volar) and omitting the extension has been deprecated by Vue 3. + + :x: **Incorrect** + + ```javascript + import MyComponent from "@/components/MyComponent" + ``` + + :white_check_mark: **Correct** + + ```javascript + import MyComponent from "@/components/MyComponent.vue" + ``` + +### Component data + +Don't use complex object (e.g. instance from an external library) in the `data` section of a component (Option API) or wrapped in a `ref` (Composition API). Refs and the data section are made deeply reactive which means that each object is recursively analysed by Vue and each object properties are proxied. This would have a negative impact on performance and might break the complex object. + +:x: BAD + +```javascript +// BAD +import { Map } from 'ol' + +export default { + data(): { + return { + myBoolean: true + myMap: new Map({ controls: [] }) + } + }, +} +``` +```javascript +import { ref } from 'vue' +import { Map } from 'ol' + +const myBoolean = ref(true) +const myMap = ref(new Map({ controls: [] })) +``` + +:white_check_mark: GOOD + +```javascript +// GOOD +import { Map } from 'ol' + +export default { + data(): { + return { + myBoolean: true + } + }, + created(): { + this.map = new Map({ controls: [] }) + }, +} +``` +```javascript +import { ref } from 'vue' +import { Map } from 'ol' + +const myBoolean = ref(true) +const myMap = new Map({ controls: [] }) +``` + + +### Component method + +#### Definition + +Don't use arrow function to define Option API component method (arrow function prevent `Vue` from binding the appropriate `this` value, see [Vue 3 - Methods](https://v3.vuejs.org/guide/data-methods.html#methods)) + +:x: BAD + +```javascript +// BAD + +const app = Vue.createApp({ + data() { + return {count: 0} + }, + methods: { + increment: () => { + // `this` don't necessarily refer to the component depending on the context + this.count++ + } + } +}) +``` + +:white_check_mark: GOOD + +```javascript +// GOOD + +const app = Vue.createApp({ + data() { + return {count: 0} + }, + methods: { + increment() { + // `this` always refer to the component + this.count++ + } + } +}) +``` + +Also prefer the short syntax `increment() {}` to `increment: function() {}` + +#### Method called from template + +Methods called from a template (except for event listeners) should not have any side effects, such as changing data or triggering asynchronous processes. If you find yourself tempted to do that you should probably use a [lifecycle hook](https://v3.vuejs.org/guide/instance.html#lifecycle-hooks) instead. For more infos about this see [Vue 3 - Methods](https://v3.vuejs.org/guide/data-methods.html#methods). + +## Debugging on Mobile Phone + +Debugging issue that only touch Mobile Phones, especially IPhones, can be quite difficult without a Mac. +For this task you can use [inspect.dev](https://inspect.dev) which works quite well. + +This tool requires a subscriptions for troubleshooting on IOS devices, currently the following user +have a yearly subscription: + +- Brice Schaffner (private subscription) diff --git a/PYTHON.md b/PYTHON.md new file mode 100644 index 0000000..6b17749 --- /dev/null +++ b/PYTHON.md @@ -0,0 +1,782 @@ +# Python coding guidelines + +There are a number of style guides out there. If in doubt, check what [google](http://google.github.io/styleguide/pyguide.html#doc-function-raises) proposes. We largely follow their guidelines. A few important things are reproduced here. + +- [1. Linting / Auto-formatting](#1-linting--auto-formatting) + - [Formatting with Yapf](#formatting-with-yapf) + - [Linting with pylint](#linting-with-pylint) + - [Ignore linting warning/refactoring/convention](#ignore-linting-warningrefactoringconvention) + - [Yapf and Pylint IDE Integration](#yapf-and-pylint-ide-integration) + - [Visual Studio Code](#visual-studio-code) + - [PyCharm](#pycharm) +- [2. Naming conventions](#2-naming-conventions) +- [3. Comments and Docstrings](#3-comments-and-docstrings) + - [Docstrings](#docstrings) + - [Modules](#modules) + - [Functions and Methods](#functions-and-methods) + - [Classes](#classes) + - [Block and Inline Comments](#block-and-inline-comments) + - [Punctuation, Spelling, and Grammar](#punctuation-spelling-and-grammar) +- [4. TODO Comments](#4-todo-comments) +- [5. Imports formatting](#5-imports-formatting) + - [.isort.cfg](#isortcfg) +- [6. Exceptions](#6-exceptions) + - [Definition](#definition) + - [Pros](#pros) + - [Cons](#cons) + - [Decision](#decision) +- [7. Error handling - Rules of Thumb](#7-error-handling---rules-of-thumb) +- [8. Introduce Explaining Variable](#8-introduce-explaining-variable) +- [9. Unit Testing frameworks](#9-unit-testing-frameworks) +- [10. Logging](#10-logging) + - [Logger](#logger) + - [Configuration](#configuration) + - [Flask logging configuration](#flask-logging-configuration) + - [Django logging configuration](#django-logging-configuration) + - [Gunicorn logging configuration](#gunicorn-logging-configuration) + +**The foremost goal is that reading and understanding your python code is easy for someone else (or yourself in a few months time).** + +## 1. Linting / Auto-formatting + +### Formatting with Yapf + +Most of the current formatters for Python --- e.g., autopep8, and pep8ify --- are made to remove lint errors from code. This has some obvious limitations. For instance, code that conforms to the PEP 8 guidelines may not be reformatted. But it doesn't mean that the code looks good. + +[YAPF](https://github.com/google/yapf/) takes a different approach. In essence, the algorithm takes the code and reformats it to the best formatting that conforms to the style guide, even if the original code didn't violate the style guide: end all holy wars about formatting - if the whole codebase of a project is simply piped through YAPF whenever modifications are made, the style remains consistent throughout the project and there's no point arguing about style in every code review. + +The ultimate goal is that the code YAPF produces is as good as the code that a programmer would write if they were following the style guide. It takes away some of the drudgery of maintaining your code. + +We use the `google` style with a few small modifications. Simply copy-paste this to the root of every new project. + +```ini +[style] +based_on_style=google +# Put closing brackets on a separate line, dedent, if the bracketed +# expression can't fit in a single line. Applies to all kinds of brackets, +# including function definitions and calls. For example: +# +# config = { +# 'key1': 'value1', +# 'key2': 'value2', +# } # <--- this bracket is dedent and on a separate line +# +# time_series = self.remote_client.query_entity_counters( +# entity='dev3246.region1', +# key='dns.query_latency_tcp', +# transform=Transformation.AVERAGE(window=timedelta(seconds=60)), +# start_ts=now()-timedelta(days=3), +# end_ts=now(), +# ) # <--- this bracket is dedent and on a separate line +dedent_closing_brackets=True +coalesce_brackets=True + +# This avoid issues with complex dictionary +# see https://github.com/google/yapf/issues/392#issuecomment-407958737 +indent_dictionary_value=True +allow_split_before_dict_value=False + +# Split before arguments, but do not split all sub expressions recursively +# (unless needed). +split_all_top_level_comma_separated_values=True + +# Split lines longer than 100 characters (this only applies to code not to +# comment and docstring) +column_limit=100 +``` + +### Linting with pylint + +Although formatting is good, it doesn't check for syntax errors nor for non pythonic idioms or bad code practice. +Therefore we also use a linter. There are several linter on the market (pylint, flake8, bandit, ...), we use +[pylint](http://pylint.pycqa.org/en/stable/) because it cover most of the errors and also has the advantage to be able +to disable rules by alias instead of by code (e.g. `# pylint: disable=unused-import` instead of `# noqa: F401` for flake8). + +We use the following `pylint` configuration: [.pylintrc](assets/.pylintrc) + +#### Ignore linting warning/refactoring/convention + +There might be good reason to disable locally some linting messages. When doing this, the reason why we disable a rule +should be documented next to the disable pragma. Disable pragma can be entered as following: + +- single line + +```python +a, b = ... # pylint: disable=unbalanced-tuple-unpacking +``` + +- single scope + +```python +def test(): + # Disable all the no-member violations in this function + # pylint: disable=no-member + ... +``` + +- block + +```python +def test(self): + ... + if self.bla: + # Disable all line-too-long in this if block + # pylint: disable=line-too-long + print('This block contain very very long lines that we explicitly don\'t want to split into several lines to match the 100 characters max line length rule') + ... +``` + +For more detail in ignoring pylint issues see +[Pylint Messages Control](http://pylint.pycqa.org/en/stable/user_guide/message-control.html) + +### Yapf and Pylint IDE Integration + +### Visual Studio Code + +To integrate `yapf` and `pylint` into Visual Studio Code simply add the following settings into the user or workspace +settings: + +```json +{ + "editor.formatOnSave": true, + "files.trimTrailingWhitespace": true, + "files.autoSave": "onFocusChange", + "python.pythonPath": ".venv/bin/python", + "python.formatting.provider": "yapf", + "python.linting.enabled": true, + "python.linting.pylintEnabled": true, + "python.linting.ignorePatterns": [ + ".vscode/*.py", + "**/site-packages/**/*.py", + ".venv", + "build" + ] +} +``` + +### PyCharm + +[How to setup](https://www.jetbrains.com/help/pycharm/configuring-third-party-tools.html) + +## 2. Naming conventions + +Python code must follow these naming conventions: + +- module: snake_case +- constant: UPPER_CASE +- variable: snake_case +- function/method: snake_case +- argument: snake_case +- class: PascalCase +- attribute: snake_case + +These naming conventions are checked by `pylint` (see `*-naming-style` keys in [pylintrc](assets/.pylintrc#L222)) + +## 3. Comments and Docstrings + +Be sure to use the right style for module, function, method docstrings and inline comments. + +### Docstrings + +Python uses docstrings to document code. A docstring is a string that is the first statement in a package, module, class or function. These strings can be extracted automatically through the __doc__ member of the object and are used by pydoc. (Try running pydoc on your module to see how it looks.) Always use the three double-quote `"""` format for docstrings (per PEP 257). A docstring should be organized as a summary line (one physical line) terminated by a period, question mark, or exclamation point, followed by a blank line, followed by the rest of the docstring starting at the same cursor position as the first quote of the first line. There are more formatting guidelines for docstrings below. + +### Modules + +Every file should contain license boilerplate. Choose the appropriate boilerplate for the license used by the project (for example, Apache 2.0, BSD, LGPL, GPL) + +Files should start with a docstring describing the contents and usage of the module. + +```python +"""A one line summary of the module or program, terminated by a period. + +Leave one blank line. The rest of this docstring should contain an +overall description of the module or program. Optionally, it may also +contain a brief description of exported classes and functions and/or usage +examples. + + Typical usage example: + + foo = ClassFoo() + bar = foo.FunctionBar() +""" +``` + +### Functions and Methods + +In this section, "function" means a method, function, or generator. + +A function must have a docstring, unless it meets all of the following criteria: + +- not externally visible +- very short +- obvious + +A docstring should give enough information to write a call to the function without reading the function’s code. The docstring should be descriptive-style (`"""Fetches rows from a Bigtable."""`) rather than imperative-style (`"""Fetch rows from a Bigtable."""`), except for @property data descriptors, which should use the same style as attributes. A docstring should describe the function’s calling syntax and its semantics, not its implementation. For tricky code, comments alongside the code are more appropriate than using docstrings. + +A method that overrides a method from a base class may have a simple docstring sending the reader to its overridden method’s docstring, such as `"""See base class."""`. The rationale is that there is no need to repeat in many places documentation that is already present in the base method’s docstring. However, if the overriding method’s behavior is substantially different from the overridden method, or details need to be provided (e.g., documenting additional side effects), a docstring with at least those differences is required on the overriding method. + +Certain aspects of a function should be documented in special sections, listed below. Each section begins with a heading line, which ends with a colon. All sections other than the heading should maintain a hanging indent of two or four spaces (be consistent within a file). These sections can be omitted in cases where the function’s name and signature are informative enough that it can be aptly described using a one-line docstring. + +**Args:** + List each parameter by name. A description should follow the name, and be separated by a colon and a newline. Optionally can the type + of parameter be specified next to the name after the colon separator. If the description is too long to fit on a single 80-character line, use a hanging indent of 4 spaces. The description should include required type(s) if the code does not contain a corresponding type annotation. If a function accepts `*foo` (variable length argument lists) and/or `**bar` (arbitrary keyword arguments), they should be listed as `*foo` and `**bar`. + +**Returns:** (or Yields: for generators) + Describe the type and semantics of the return value. If the function only returns None, this section is not required. It may also be omitted if the docstring starts with Returns or Yields (e.g. """Returns row from Bigtable as a tuple of strings.""") and the opening sentence is sufficient to describe return value. + +**Raises:** + List all exceptions that are relevant to the interface. You should not document exceptions that get raised if the API specified in the docstring is violated (because this would paradoxically make behavior under violation of the API part of the API). + +```python +def fetch_bigtable_rows(big_table, keys, other_silly_variable=None): + """Fetches rows from a Bigtable. + + Retrieves rows pertaining to the given keys from the Table instance + represented by big_table. Silly things may happen if + other_silly_variable is not None. + + Args: + big_table: + An open Bigtable Table instance. + keys: list + A sequence of strings representing the key of each table row + to fetch. + other_silly_variable: string + Another optional variable, that has a much + longer name than the other args, and which does nothing. + + Returns: + A dict mapping keys to the corresponding table row data + fetched. Each row is represented as a tuple of strings. For + example: + + {'Serak': ('Rigel VII', 'Preparer'), + 'Zim': ('Irk', 'Invader'), + 'Lrrr': ('Omicron Persei 8', 'Emperor')} + + If a key from the keys argument is missing from the dictionary, + then that row was not found in the table. + + Raises: + IOError: An error occurred accessing the bigtable.Table object. + """ +``` + +### Classes + +Classes should have a docstring below the class definition describing the class. If your class has public attributes, they should be documented here in an Attributes section and follow the same formatting as a function’s Args section. + +```python +class SampleClass(object): + """Summary of class here. + + Longer class information.... + Longer class information.... + + Attributes: + likes_spam: + A boolean indicating if we like SPAM or not. + eggs: + An integer count of the eggs we have laid. + """ + + def __init__(self, likes_spam=False): + """Inits SampleClass with blah.""" + self.likes_spam = likes_spam + self.eggs = 0 + + def public_method(self): + """Performs operation blah.""" +``` + +### Block and Inline Comments + +The final place to have comments is in tricky parts of the code. Comments should not document the what, but the why: What the code does, the code itself should describe in a readable and comprehensible way, but why the code was written in a certain way and what trade-offs were made should be written in a comment. If you’re going to have to explain it at the next code review, you should comment it now. Complicated operations get a few lines of comments before the operations commence. Non-obvious ones get comments at the end of the line. + +```python +# We use a weighted dictionary search to find out where i is in +# the array. We extrapolate position based on the largest num +# in the array and the array size and then do binary search to +# get the exact number. + +if i & (i-1) == 0: # True if i is 0 or a power of 2. +``` + +To improve legibility, these comments should start at least 2 spaces away from the code with the comment character `#`, followed by at least one space before the text of the comment itself. + +On the other hand, never describe the code. Assume the person reading the code knows Python (though not what you’re trying to do) better than you do. + +```python +# BAD COMMENT: Now go through the b array and make sure whenever i occurs +# the next element is i+1 +``` + +### Punctuation, Spelling, and Grammar + +Pay attention to punctuation, spelling, and grammar; it is easier to read well-written comments than badly written ones. + +Comments should be as readable as narrative text, with proper capitalization and punctuation. In many cases, complete sentences are more readable than sentence fragments. Shorter comments, such as comments at the end of a line of code, can sometimes be less formal, but you should be consistent with your style. + +Although it can be frustrating to have a code reviewer point out that you are using a comma when you should be using a semicolon, it is very important that source code maintain a high level of clarity and readability. Proper punctuation, spelling, and grammar help with that goal. + +## 4. TODO Comments + +Use `TODO` comments for code that is temporary, a short-term solution, or good-enough but not perfect. + +A `TODO` comment begins with the string `TODO` in all caps and the abbreviation (e.g. boc) or other identifier of the person or issue with the best context about the problem. This is followed by an explanation of what there is to do. + +The purpose is to have a consistent `TODO` format that can be searched to find out how to get more details. A `TODO` is not a commitment that the person referenced will fix the problem. Thus when you create a `TODO`, it is almost always your name that is given. + +```python +# TODO(kl@gmail.com): Use a "*" here for string repetition. +# TODO(boc) Change this to use relations. +``` + +If your `TODO` is of the form "At a future date do something" make sure that you either include a very specific date ("Fix by November 2009") or a very specific event ("Remove this code when all clients can handle XML responses."). + +## 5. Imports formatting + +Imports should be on separate lines. + +E.g.: + +```python +Yes: import os + import sys + +No: import os, sys +``` + +Imports are always put at the top of the file, just after any module comments and docstrings and before module globals and constants. Imports should be grouped from most generic to least generic: + +1. Python standard library imports. For example: + + ```python + import sys + ``` + +1. third-party module or package imports. For example: + + ```python + import tensorflow as tf + ``` + +1. Code repository sub-package imports. For example: + + ```python + from otherproject.ai import mind + ``` + +1. application-specific imports that are part of the same top level sub-package as this file. For example: + + ```python + from myproject.backend.hgwells import time_machine + ``` + +Within each grouping, imports should be sorted lexicographically, ignoring case, according to each module’s full package path. Code may optionally place a blank line between import sections. + +```python +import collections +import queue +import sys + +from absl import app +from absl import flags +import bs4 +import cryptography +import tensorflow as tf + +from book.genres import scifi +from otherproject.ai import body +from otherproject.ai import mind +from otherproject.ai import soul + +from myproject.backend.hgwells import time_machine +from myproject.backend.state_machine import main_loop +``` + +[`isort`](https://timothycrosley.github.io/isort/) does the job of grouping and sorting your imports alphabetically and according to your configuration. If you have packages that you want to have treated as thirdparty, add them to `known_third_party`. If you wanna specially group imports from e.g. a framework, you can create a custom `known_acme` and add `ACME` to the `sections` + +### .isort.cfg + +```ini +[settings] +known_third_party=pytest +known_django=django +known_flask=flask +force_single_line=True +sections=FUTURE,STDLIB,THIRDPARTY,DJANGO,FLASK,FIRSTPARTY,LOCALFOLDER + +# other possible options +# line_length=80 +# force_to_top=file1.py,file2.py +# skip=file3.py,file4.py +# known_future_library=future,pies +# known_standard_library=std,std2 +# known_third_party=randomthirdparty +# known_first_party=mylib1,mylib2 +# indent=' ' +# multi_line_output=0 # 0 is default +# length_sort=1 +# forced_separate=django.contrib,django.utils +# default_section=FIRSTPARTY +# no_lines_before=LOCALFOLDER + +``` + +## 6. Exceptions + +Exceptions are allowed but must be used carefully. + +### Definition + +Exceptions are a means of breaking out of the normal flow of control of a code block to handle errors or other exceptional conditions. + +### Pros + +The control flow of normal operation code is not cluttered by error-handling code. It also allows the control flow to skip multiple frames when a certain condition occurs, e.g., returning from N nested functions in one step instead of having to carry-through error codes. + +### Cons + +May cause the control flow to be confusing. Easy to miss error cases when making library calls. + +### Decision + +Exceptions must follow certain conditions: + +- Raise exceptions like this: `raise MyError('Error message')` or `raise MyError()`. Do not use the two-argument form (`raise MyError, 'Error message'`). + +- Make use of built-in exception classes when it makes sense. For example, raise a `ValueError` to indicate a programming mistake like a violated precondition (such as if you were passed a negative number but required a positive one). Do not use assert statements for validating argument values of a public API. assert is used to ensure internal correctness, not to enforce correct usage nor to indicate that some unexpected event occurred. If an exception is desired in the latter cases, use a raise statement. For example: + + ```python + # Yes: + def connect_to_next_port(self, minimum): + """Connects to the next available port. + + Args: + minimum: A port value greater or equal to 1024. + + Returns: + The new minimum port. + + Raises: + ConnectionError: If no available port is found. + """ + if minimum < 1024: + # Note that this raising of ValueError is not mentioned in the doc + # string's "Raises:" section because it is not appropriate to + # guarantee this specific behavioral reaction to API misuse. + raise ValueError('Minimum port must be at least 1024, not %d.' % (minimum,)) + port = self._find_next_open_port(minimum) + if not port: + raise ConnectionError('Could not connect to service on %d or higher.' % (minimum,)) + assert port >= minimum, 'Unexpected port %d when minimum was %d.' % (port, minimum) + return port + ``` + + ```python + # No: + def connect_to_next_port(self, minimum): + """Connects to the next available port. + + Args: + minimum: A port value greater or equal to 1024. + + Returns: + The new minimum port. + """ + assert minimum >= 1024, 'Minimum port must be at least 1024.' + port = self._find_next_open_port(minimum) + assert port is not None + return port + ``` + +- Libraries or packages may define their own exceptions. When doing so they must inherit from an existing exception class. Exception names should end in `Error` and should not introduce stutter (foo.FooError). +- Never use catch-all except: statements, or catch `Exception` or `StandardError` (see also [7. Error handling - Rules of Thumb](#7-error-handling---rules-of-thumb)), unless you are + - re-raising the exception, or + - creating an isolation point in the program where exceptions are not propagated but are recorded and suppressed instead, such as protecting a thread from crashing by guarding its outermost block + + Catch-all exception is check by `pylint` and is configured via `overgeneral-exceptions` in [.pylintrc](assets/.pylintrc#L526). + Python is very tolerant in this regard and except: will really catch everything including misspelled names, `sys.exit()` calls, `Ctrl+C` interrupts, unittest failures and all kinds of other exceptions that you simply don’t want to catch. +- Minimize the amount of code in a `try/except` block. The larger the body of the try, the more likely that an exception will be raised by a line of code that you didn’t expect to raise an exception. In those cases, the `try/except` block hides a real error. +- Use the `finally` clause to execute code whether or not an exception is raised in the try block. This is often useful for cleanup, i.e., closing a file. +- When capturing an exception, use `as` rather than a comma. For example: + + ```python + try: + raise Error() + except Error as error: + pass + ``` + +- Every exception must also be logged with the correct severity level; `CRITICAL`. +- When re-raising exception, in order to have a comprehensible backtrace always use `raise ... from ...` form. There is two use cases with the `from`: + 1. The new exception contains all useful information and/or the exception is meant to be anyway handle later on (e.g. Django `ValidationError` exception). In this case in order to have consice backtrace uses `from None` + + ```python + def get_value(my_dict, key): + try: + return my_dict[key] + except KeyError as error: + raise KeyError(f'Key {key} missing from my_dict') from None + + # This would result to such backtrace + >>> get_value({}, 'my_key') + Traceback (most recent call last): + File "", line 1, in + File "", line 5, in get_value + KeyError: 'Key my_key missing from my_dict' + + # instead of + >>> get_value({}, 'my_key') + Traceback (most recent call last): + File "", line 3, in get_value + KeyError: 'my_key' + + During handling of the above exception, another exception occurred: + + Traceback (most recent call last): + File "", line 1, in + File "", line 5, in get_value + KeyError: 'Key my_key missing from my_dict' + ``` + + 1. The original exception still contains useful information, therefore use `from error` to have the original and new backtrace + + ```python + def do_something(): + try: + raise ValueError('Original error') + except ValueError as error: + raise RuntimeError('This should not happen') from error + + # This generate the following backtrace + >>> do_something() + Traceback (most recent call last): + File "", line 3, in do_something + ValueError: Original error + + The above exception was the direct cause of the following exception: + + Traceback (most recent call last): + File "", line 1, in + File "", line 5, in do_something + RuntimeError: This should not happen + + # Instead of + >>> do_something() + Traceback (most recent call last): + File "", line 3, in do_something + ValueError: Original error + + During handling of the above exception, another exception occurred: + + Traceback (most recent call last): + File "", line 1, in + File "", line 5, in do_something + RuntimeError: This should not happen + # As you noticed the message is slightly different.. + ``` + + For more information about `raise ... from ...` form see [Raise … from … in Python](https://stefan.sofa-rockers.org/2020/10/28/raise-from/) + +## 7. Error handling - Rules of Thumb + +- Only handle known _Exceptions_ -> **NO BROAD EXCEPTION !** + + ```python + # NEVER DO THIS ! + try: + return something() + except: + return '' + + # NOR THIS ! + try: + return something() + except Exception: + return '' + ``` + +- Only handle _Exceptions_ if you know how to fix it + + ```python + # DON'T DO THIS ! + try: + return something(param) + except ValueError: + return '' + + # But you can do this + try: + return True, something(param) + except ValueError as err: + logger.error('invalid param: %s', err) + return False, 'invalid param: %s' % (err) + ``` + +- Always log backtrace for unexpected _Exceptions_ + + ```python + try: + return something() + except Exception as err: + logger.exception(err) + raise + ``` + +- Always use `from` when re-raising new exception + + ```python + # When the re-raise exception contains all information use `from None` + try: + return something() + except KeyError as err: + raise KeyError(f'Key {err} is missing') from None + + # or when original exception contains useful information + try: + return something(param) + except ValueError as err: + raise MyException('Invalid parameter') from err + ``` + +- Let higher level application handle unexpected _Exceptions_ whenever possible + - Flask and Django handles all unexpected _Exceptions_ with logging backtrace and returns a `500, Internal Server Error` + +- Let crash the application with unexpected _Exceptions_ rather sooner than later + +## 8. Introduce Explaining Variable + +This will help to explain the meaning of each variable when expressions are hard to read. + +```python +# maybe not the most illustrative example, but you get the idea +# change +if ( "MAC" in platform.upper() and \ + "IE" in browser.upper() and \ + was_initialized() and \ + resize > 0 ): + # do something + +# to +is_mac_os = "MAC" in platform.upper() +is_IEBrowser = "IE" in browser.upper() +was_resized = resize > 0 +if (is_mac_os and is_IEBrowser and was_initialized() and was_resized): + # do something +``` + +## 9. Unit Testing frameworks + +Python comes with a fairly mature unit testing framework [`unittest`](https://docs.python.org/3/library/unittest.html) that should be used. There are a number of different test runners available: + +- nose2 (successor of `nose` which isn't developed anymore) +- pytest +- various test runners included in frameworks + +In any case, the tests should be based on `unittest`. + +## 10. Logging + +Python comes with a good logging framework that we should use for logging. Unfortunately this framework lack for good JSON formatting which is the best logging format when working with ELK (Elasticsearch-Logstash-Kibana). Therefore we use the [`logging-utilities`](https://pypi.org/project/logging-utilities/) library for JSON support, Flask Request extension and ISO time. + +### Logger + +We should always use a logger per module named after it as follow: + +```python +# my_module.py + +import logging + +logger = logging.getLogger(__name__) + +logger.info('This is my module logger') +``` + +**NOTE:** don't reuse the Flask app logger. + +### Configuration + +Logging should be configured via a yaml file as follow: + +```python +import logging +import logging.config +import os + +import yaml + + +logger = logging.getLogger(__name__) + +def get_logging_cfg(): + cfg_file = os.getenv('LOGGING_CFG', 'logging-cfg-local.yml') + + config = {} + with open(cfg_file, 'rt') as fd: + config = yaml.safe_load(fd.read()) + + logger.debug('Load logging configuration from file %s', cfg_file) + return config + + +def init_logging(): + config = get_logging_cfg() + logging.config.dictConfig(config) +``` + +Each application should use the following configurations depending on the environment: + +- [logging-cfg-local.yml](assets/logging-cfg-local.yml) (local development) +- [logging-cfg-dev.yml](assets/logging-cfg-dev.yml) +- [logging-cfg-prod.yml](assets/logging-cfg-prod.yml) + +#### Flask logging configuration + +When serving flask directly (e.g. `make serve`) we need to configure logging by calling the `init_logging()` method insid the `service_launcher.py` file (see [geoadmin/template-service-flask/service_launcher.py](https://github.com/geoadmin/template-service-flask/blob/master/service_launcher.py)) + +#### Django logging configuration + +When serving Django directly (e.g. `make serve` without WSGI server), we need to set the logging configuration in `project/settings.py` as follow: + +```python +# Logging +# https://docs.djangoproject.com/en/3.1/topics/logging/ + + +# Read configuration from file +def get_logging_config(): + '''Read logging configuration + + Read and parse the yaml logging configuration file passed in the environment variable + LOGGING_CFG and return it as dictionary + ''' + log_config = {} + with open(os.getenv('LOGGING_CFG', 'logging-cfg-local.yml'), 'rt') as fd: + log_config = yaml.safe_load(fd.read()) + return log_config + + +LOGGING = get_logging_config() +``` + +#### Gunicorn logging configuration + +To configure gunicorn logging you need to set the `logconfig_dict` config with the output of `get_logging_cfg()` + +```python +# We use the port 5000 as default, otherwise we set the HTTP_PORT env variable within the container. +if __name__ == '__main__': + HTTP_PORT = str(os.environ.get('HTTP_PORT', "5000")) + # Bind to 0.0.0.0 to let your app listen to all network interfaces. + options = { + 'bind': '%s:%s' % ('0.0.0.0', HTTP_PORT), + 'worker_class': 'gevent', + 'workers': 2, # scaling horizontally is left to Kubernetes + 'timeout': 60, + 'logconfig_dict': get_logging_config() + } + StandaloneApplication(application, options).run() +``` diff --git a/PYTHON_PACKAGE.md b/PYTHON_PACKAGE.md new file mode 100644 index 0000000..bc8f931 --- /dev/null +++ b/PYTHON_PACKAGE.md @@ -0,0 +1,207 @@ +# Python Packaging + +Some python tool, framework or library might be published on PyPI index. This way they can be easily shared within projects or within the community. + +## Table of Content + +- [Table of Content](#table-of-content) +- [Create python package](#create-python-package) +- [License](#license) +- [setup.py](#setuppy) +- [Makefile](#makefile) +- [Publishing](#publishing) + +## Create python package + +Python packages should be created using the official [PyPA](https://packaging.python.org/tutorials/packaging-projects/) best practices. + +The following rules needs to be applied: + +- version should follow [Semantic Versioning 2.0.0](https://semver.org/) + - NOTE: version 0.x.x are for unstable release and don't appears in pypi search +- version should be placed in the `/__init__.py` file as follow + + ```python + VERSION = (0, 1, 0, 'alpha0') + if isinstance(VERSION[-1], str): + # Support for alpha version: 0.1.0-alpha1 + __version__ = "-".join([".".join(map(str, VERSION[:-1])), VERSION[-1]]) + else: + __version__ = ".".join(map(str, VERSION)) + ``` + + **NOTE:** remove the `'alpha0'` string in the `VERSION` tuple for official version + +- version should be read from `/__init__.py` in the _setup.py_ file +- long description in _setup.py_ should be read from _README.md_ +- package MUST have a license +- license should be _BSD 3-Clause License_ (see [License](#license)) +- package should have unittest +- package should be verified by a travis project + +## License + +```text +BSD 3-Clause License + +Copyright (c) 2020, swisstopo +All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are met: + +1. Redistributions of source code must retain the above copyright notice, this +list of conditions and the following disclaimer. + +2. Redistributions in binary form must reproduce the above copyright notice, +this list of conditions and the following disclaimer in the documentation +and/or other materials provided with the distribution. + +3. Neither the name of the copyright holder nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE +FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +``` + +## setup.py + +Here below an example of the _setup.py_ file + +```python +#!/usr/bin/env python + +from setuptools import setup, find_packages + +# Get description from README.md +LONG_DESCRIPTION = '' +with open('README.md', encoding='utf-8') as rd: + LONG_DESCRIPTION = rd.read() + +# Here we cannot import the version but need to read it otherwise we might have an ImportError +# during execution of the setup.py if the package contains other libraries. +VERSION_LINE = list(filter(lambda l: l.startswith('VERSION'), + open('./logging_utilities/__init__.py')))[0] + + +def get_version(version_line): + # pylint: disable=eval-used + version_tuple = eval(version_line.split('=')[-1]) + return ".".join(map(str, version_tuple)) + + +setup( + name='logging-utilities', + version=get_version(VERSION_LINE), + description=('Python logging utilities'), + long_description=LONG_DESCRIPTION, + long_description_content_type="text/markdown", + platforms=["all"], + classifiers=[ + 'Development Status :: 3 - Alpha', + 'Intended Audience :: Developers', + 'License :: OSI Approved :: BSD License', + 'Operating System :: OS Independent', + 'Programming Language :: Python :: 3', + 'Programming Language :: Python :: 3 :: Only', + 'Topic :: Utilities', + 'Topic :: System :: Logging' + ], + python_requires='>=3.0', + author='ltshb', + author_email='brice.schaffner@swisstopo.ch', + url='https://github.com/geoadmin/lib-py-logging-utilities', + license='BSD 3-Clause License', + packages=find_packages() +) +``` + +## Makefile + +In order to simplify the packaging and publishing the following makefile targets should be added + +```makefile +# general targets timestamps +TIMESTAMPS = .timestamps +PREP_PACKAGING_TIMESTAMP = $(TIMESTAMPS)/.prep-packaging.timestamp + +# PyPI credentials +PYPI_USER ?= +PYPI_PASSWORD ?= + +# Get package version from package info file +PACKAGE_VERSION = $(shell awk '/^Version:/ {print $$2}' logging_utilities.egg-info/PKG-INFO) + +.PHONY: help +help: + @echo "Usage: make " + @echo + @echo "Possible targets:" + @echo -e " \033[1mPACKAGING TARGETS\033[0m " + @echo "- package Create package" + @echo "- publish Tag and publish package to PyPI" + @echo -e " \033[1mCLEANING TARGETS\033[0m " + @echo "- clean Clean generated files" + +# Packaging target + +.PHONY: package +package: $(PREP_PACKAGING_TIMESTAMP) + python3 setup.py sdist bdist_wheel + + +.PHONY: publish +publish: publish-check clean test package + @echo "Tag and upload package version=$(PACKAGE_VERSION)" + @# Check if we use interactive credentials or not + @if [ -n "$(PYPI_PASSWORD)" ]; then \ + python3 -m twine upload -u $(PYPI_USER) -p $(PYPI_PASSWORD) dist/*; \ + else \ + python3 -m twine upload dist/*; \ + fi + git tag -am $(PACKAGE_VERSION) $(PACKAGE_VERSION) + git push origin $(PACKAGE_VERSION) + + +.PHONY: clean +clean: + rm -rf $(TIMESTAMPS) + rm -rf dist + rm -rf build + rm -rf *.egg-info + + +$(TIMESTAMPS): + mkdir -p $(TIMESTAMPS) + +$(PREP_PACKAGING_TIMESTAMP): $(TIMESTAMPS) + python3 -m pip install --user --upgrade setuptools wheel twine + @touch $(PREP_PACKAGING_TIMESTAMP) + + +publish-check: + @echo "Check if publish is allowed" + @if [ -n "`git status --porcelain`" ]; then echo "ERROR: Repo is dirty !" >&2; exit 1; fi + @# "Check if TAG=${PACKAGE_VERSION} already exits" + @if [ -n "`git ls-remote --tags --refs origin refs/tags/${PACKAGE_VERSION}`" ]; then \ + echo "ERROR: Tag ${PACKAGE_VERSION} already exists on remote" >&2; \ + exit 1; \ + fi +``` + +## Publishing + +In order to publish to PyPI index we use the *pypi/swisstopo* user. This user can be found in _gopass_ and you can use _summon_ to publish the package as follow: + +```shell +summon -p gopass make publish +``` diff --git a/README.md b/README.md index 2f4ae96..290c648 100644 --- a/README.md +++ b/README.md @@ -1,2 +1,16 @@ # doc-guidelines -Geoadmin BGDI coding guidelines. Managed by geoadmin/infra-terraform-github-bgdi + +Geoadmin PP BGDI general and coding guidelines. + +## Table of Content + +1. Coding guidelines + 1. [Ansible](ANSIBLE.md) + 2. [Bash](BASH.md) + 3. [Docker](DOCKER.md) + 4. [Javascript](JAVASCRIPT.md) + 5. [Python](PYTHON.md) + 6. [Python packaging](PYTHON_PACKAGE.md) +2. Github guidelines + 1. [Repository naming convention](GITHUB_NAMES.md) + 2. [Git cheat sheet](GIT_CHEAT_SHEET.md)