seravo / gnitpick Goto Github PK

If Gnitpick runs on a system that does not have git installed, there will be an ugly Python error and trace. Implement proper check for binary git and error with a good error message telling users to apt install git etc.

The pre-commit.com project is a framework for automatic code quality assurance running at git commit time, or using other available hooks in git.

Research how to integrate Gnitpick with them and get listed at https://pre-commit.com/hooks.html

Currently the requirement to have @seravo in author email is hard-coded, which naturally will not work for many people :)

This should be an option via argparse (see #1) or something that can be configured in a .gnitpick config file.

Currently Gnitpick does not complain anything about a commit like:

 Gnitpick inspecting git revisions range origin/master..HEAD
  commit 59705a9aa1ce99b5cfc7c1617f1f10bed29d9934
  Author: HACKING-STATION\gunhaxxor <[email protected]>
  Date:   Tue Apr 21 12:19:02 2020 +0200
  
      Fixed issue #131
 Inspecting commit 59705a9aa1ce99b5cfc7c1617f1f10bed29d9934:
 Gnitpick commit inspection passed!

Ideally it would check that author name looks like a real name.

Also it would be cool to detect past tense from the git title and enforce imperative form.

An user should be able to run gnitpick on their own repo via github actions. This should be as easy as possible and the default parameters should be generic and not too strict.

We should first try this out with Seravo's own projects and later maybe even publish the action.

Gnitpick has enough features now that there is a risk for regressions. A test suite should be written.

..for easy use on Github.

Mostly it is just a question of implementing a https://github.com/Seravo/action-wordpress-plugin-asset-update/blob/develop/action.yml file.

If users make a mistake, the error message should be move verbose and have a link to some git documentation so users can read more about best practices.

This makes it easier to invoke on the command line and have some parameters sent to the command, instead of having it read environment variables etc.

Currently the script is hard-coded to only run on Travis-CI and read Travis-CI environment variables. Refactor this script so that it can also be run locally. Issue #1 needs to be solved first so that this one becomes possible.

Current codebase assumes there is a origin/master somewhere. The origin might have some other name, such as 'upstream' and also the master branch might be 'trunk', 'mainline', 'devel' etc. This should be configurable as command arguments.

Idea from discussion with @ericherman

The Commitzen project helps enforce git commit messages with a special focus on enforcing developers to adhere to the Angular type of commit practices, where each commit is prefixed with a commit category name. Personally I dislike this practice and think commit subjects should not have a category enforced not should changelogs be automatically exported from commit logs (I think changelogs should be human curated text conveying a message to other humans, not just a plain list).

It might however have some nice features, so we should compare their features at https://commitizen-tools.github.io/commitizen/customization/

We had a problem where dependabot makes a commit automatically for dependency update. The commit failed gnitpick tests with the following:

Author name 'dependabot[bot]' does not start with an uppercase letter

It would be nice to have a whitelist of common bots or something other logic like if "bot" in name => ok. Please consider the pros and cons of different solutions.

Gnitpick was initially written with Github and Travis-CI in mind. But if might also be useful on Gitlab-CI.

We should incorporate checking for CI_MERGE_REQUEST_SOURCE_BRANCH_SHA and CI_MERGE_REQUEST_TARGET_BRANCH_SHA to get the Gitlab-CI Merge Request commit range.

See https://docs.gitlab.com/ee/ci/variables/predefined_variables.html

Related to the current "merge request should not include merges itself", merges themselves inside to be merged new branch are not necessarily bad, but what specifically is bad, is "diamond" structures where there is "parallel" history that is difficult to bisect.

Idea from discussion with @ericherman

Work-in-progress commits are a good practice for temporary commits during development, but WIP commits should never land on the master branch. Some CI systems (e.g. Gitlab) prevent automatically merges of WIP commits, but it would be good if also Gnitpick prevented those (e.g. to support Gihub users).

Some guidelines are not strict and CI systems should not fail on "small things". However developers running Gnitpick locally might want to be as good as possible and also meet the extra guidelines, which we could show with the --verbose flag (or --pedantic if we want to follow the Lintian vocabulary).

Idea from discussion with @ericherman

The Commitlint project is also about enforcing git commit message quality, and like Commitzen (see #15) it also by default enforces Angular style which I am not a fan of.

However, we should compare to the features it has documented at https://commitlint.js.org/#/reference-rules

sami@pad:~/.../seravo-plugin (master) $ gnitpick --target-branch dev-sami
Traceback (most recent call last):
  File "/usr/bin/gnitpick", line 321, in <module>
    git_rev = f'{target_repository}/{target_branch}..HEAD'
NameError: name 'target_branch' is not defined

If user defines a target_branch with --target_branch, gnitpick will not execute correctly since the correct value cannot be accessed properly. If no target branch is given, gnitpick defaults to master and the program runs correctly.

Using the value of target_branch attribute is since impossible, for instance in #20.