dx: Add pre-commit as a central linter, formatter and checker by cdecker · Pull Request #7884 · ElementsProject/lightning

cdecker · 2024-11-28T14:40:55Z

We have quite the variety of different formatters, checkers and linters:

Python formatting, code style and linting
shellcheck run via make
check-amount-access grepping for access to the u64 inside of the amount_msat and amount_sat structs.
JSON-Schema formatting
discouraged-fundctions
check-gen-updated
check-includes
- check-src-includes
- check-hdr-includes
check-hsm-versions
check-manpages
check-spelling
check-whitespace

For many of these we have custom scripts that require us to maintain
them as well, despite there being off-the-shelf tooling that allows us
to check and enfore these.

pre-commit standardizes the execution of these checkers, running
them against the proposed changes in a PR. It allows for checking and
fixing as separate steps, so CI can just check, and show the diff of
changes that would have been applied.

The --from-ref and --to-ref params help us to limit the scope of
the checks to the proposed changes, allowing us to incrementally adopt
these standards as we visit the files.

If you adopt a new file, i.e., modify a file that has not been checked
before, you might want to just quickly run pre-commit run --from-ref=master --to-ref=HEAD after the first modification, and fix
up any lints or failing checks before moving on to actually
implementing your changes. This keeps the lint cleanup separate from
logic changes that are to be reviewed.

By limiting the commits that we look at to the changes since `master` we can incrementally pull files under the coverage of these lints and checks. Changelog-None: Not applicable, this is DX

s373nZ · 2024-11-29T20:30:19Z

I see flake8 checks in the Makefile as well. Is the intention here as well to replace flake8 with ruff?

cdecker · 2024-11-30T13:37:42Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

s373nZ · 2024-11-30T13:51:50Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

I should have phrased my comment more clearly. I see the Makefile is utilizing flake8 to run checks on the Python code using the check-python-flake8 target. My question is if the intention is to replace flake8 checks with ruff or if we want to add the flake8 existing checks in an additional hook.

cdecker · 2024-12-08T10:52:04Z

Oh, I see. The intention is to use one l'Inter per language. Otherwise we might get into a territory where they disagree causing unresolvable lint warnings.

Let's remove flake8 altogether.

s373nZ · 2025-03-11T22:47:42Z

For others reviewing this PR, here is another demo PR against the original branch containing some initial precommit experiments: cdecker#7

rustyrussell · 2025-04-01T03:03:34Z

Good start! Let's merge this so we can build on it!

cdecker · 2024-11-28T14:40:55Z

We have quite the variety of different formatters, checkers and linters:

Python formatting, code style and linting
shellcheck run via make
check-amount-access grepping for access to the u64 inside of the amount_msat and amount_sat structs.
JSON-Schema formatting
discouraged-fundctions
check-gen-updated
check-includes
- check-src-includes
- check-hdr-includes
check-hsm-versions
check-manpages
check-spelling
check-whitespace

For many of these we have custom scripts that require us to maintain
them as well, despite there being off-the-shelf tooling that allows us
to check and enfore these.

pre-commit standardizes the execution of these checkers, running
them against the proposed changes in a PR. It allows for checking and
fixing as separate steps, so CI can just check, and show the diff of
changes that would have been applied.

The --from-ref and --to-ref params help us to limit the scope of
the checks to the proposed changes, allowing us to incrementally adopt
these standards as we visit the files.

If you adopt a new file, i.e., modify a file that has not been checked
before, you might want to just quickly run pre-commit run --from-ref=master --to-ref=HEAD after the first modification, and fix
up any lints or failing checks before moving on to actually
implementing your changes. This keeps the lint cleanup separate from
logic changes that are to be reviewed.

By limiting the commits that we look at to the changes since `master` we can incrementally pull files under the coverage of these lints and checks. Changelog-None: Not applicable, this is DX

s373nZ · 2024-11-29T20:30:19Z

I see flake8 checks in the Makefile as well. Is the intention here as well to replace flake8 with ruff?

cdecker · 2024-11-30T13:37:42Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

s373nZ · 2024-11-30T13:51:50Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

I should have phrased my comment more clearly. I see the Makefile is utilizing flake8 to run checks on the Python code using the check-python-flake8 target. My question is if the intention is to replace flake8 checks with ruff or if we want to add the flake8 existing checks in an additional hook.

cdecker · 2024-12-08T10:52:04Z

Oh, I see. The intention is to use one l'Inter per language. Otherwise we might get into a territory where they disagree causing unresolvable lint warnings.

Let's remove flake8 altogether.

s373nZ · 2025-03-11T22:47:42Z

For others reviewing this PR, here is another demo PR against the original branch containing some initial precommit experiments: cdecker#7

rustyrussell · 2025-04-01T03:03:34Z

Good start! Let's merge this so we can build on it!

cdecker · 2024-11-28T14:40:55Z

We have quite the variety of different formatters, checkers and linters:

Python formatting, code style and linting
shellcheck run via make
check-amount-access grepping for access to the u64 inside of the amount_msat and amount_sat structs.
JSON-Schema formatting
discouraged-fundctions
check-gen-updated
check-includes
- check-src-includes
- check-hdr-includes
check-hsm-versions
check-manpages
check-spelling
check-whitespace

For many of these we have custom scripts that require us to maintain
them as well, despite there being off-the-shelf tooling that allows us
to check and enfore these.

pre-commit standardizes the execution of these checkers, running
them against the proposed changes in a PR. It allows for checking and
fixing as separate steps, so CI can just check, and show the diff of
changes that would have been applied.

The --from-ref and --to-ref params help us to limit the scope of
the checks to the proposed changes, allowing us to incrementally adopt
these standards as we visit the files.

If you adopt a new file, i.e., modify a file that has not been checked
before, you might want to just quickly run pre-commit run --from-ref=master --to-ref=HEAD after the first modification, and fix
up any lints or failing checks before moving on to actually
implementing your changes. This keeps the lint cleanup separate from
logic changes that are to be reviewed.

By limiting the commits that we look at to the changes since `master` we can incrementally pull files under the coverage of these lints and checks. Changelog-None: Not applicable, this is DX

s373nZ · 2024-11-29T20:30:19Z

I see flake8 checks in the Makefile as well. Is the intention here as well to replace flake8 with ruff?

cdecker · 2024-11-30T13:37:42Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

s373nZ · 2024-11-30T13:51:50Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

I should have phrased my comment more clearly. I see the Makefile is utilizing flake8 to run checks on the Python code using the check-python-flake8 target. My question is if the intention is to replace flake8 checks with ruff or if we want to add the flake8 existing checks in an additional hook.

cdecker · 2024-12-08T10:52:04Z

Oh, I see. The intention is to use one l'Inter per language. Otherwise we might get into a territory where they disagree causing unresolvable lint warnings.

Let's remove flake8 altogether.

s373nZ · 2025-03-11T22:47:42Z

For others reviewing this PR, here is another demo PR against the original branch containing some initial precommit experiments: cdecker#7

rustyrussell · 2025-04-01T03:03:34Z

Good start! Let's merge this so we can build on it!

cdecker · 2024-11-28T14:40:55Z

We have quite the variety of different formatters, checkers and linters:

Python formatting, code style and linting
shellcheck run via make
check-amount-access grepping for access to the u64 inside of the amount_msat and amount_sat structs.
JSON-Schema formatting
discouraged-fundctions
check-gen-updated
check-includes
- check-src-includes
- check-hdr-includes
check-hsm-versions
check-manpages
check-spelling
check-whitespace

For many of these we have custom scripts that require us to maintain
them as well, despite there being off-the-shelf tooling that allows us
to check and enfore these.

pre-commit standardizes the execution of these checkers, running
them against the proposed changes in a PR. It allows for checking and
fixing as separate steps, so CI can just check, and show the diff of
changes that would have been applied.

The --from-ref and --to-ref params help us to limit the scope of
the checks to the proposed changes, allowing us to incrementally adopt
these standards as we visit the files.

If you adopt a new file, i.e., modify a file that has not been checked
before, you might want to just quickly run pre-commit run --from-ref=master --to-ref=HEAD after the first modification, and fix
up any lints or failing checks before moving on to actually
implementing your changes. This keeps the lint cleanup separate from
logic changes that are to be reviewed.

By limiting the commits that we look at to the changes since `master` we can incrementally pull files under the coverage of these lints and checks. Changelog-None: Not applicable, this is DX

s373nZ · 2024-11-29T20:30:19Z

I see flake8 checks in the Makefile as well. Is the intention here as well to replace flake8 with ruff?

cdecker · 2024-11-30T13:37:42Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

s373nZ · 2024-11-30T13:51:50Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

I should have phrased my comment more clearly. I see the Makefile is utilizing flake8 to run checks on the Python code using the check-python-flake8 target. My question is if the intention is to replace flake8 checks with ruff or if we want to add the flake8 existing checks in an additional hook.

cdecker · 2024-12-08T10:52:04Z

Oh, I see. The intention is to use one l'Inter per language. Otherwise we might get into a territory where they disagree causing unresolvable lint warnings.

Let's remove flake8 altogether.

s373nZ · 2025-03-11T22:47:42Z

For others reviewing this PR, here is another demo PR against the original branch containing some initial precommit experiments: cdecker#7

rustyrussell · 2025-04-01T03:03:34Z

Good start! Let's merge this so we can build on it!

cdecker · 2024-11-28T14:40:55Z

We have quite the variety of different formatters, checkers and linters:

Python formatting, code style and linting
shellcheck run via make
check-amount-access grepping for access to the u64 inside of the amount_msat and amount_sat structs.
JSON-Schema formatting
discouraged-fundctions
check-gen-updated
check-includes
- check-src-includes
- check-hdr-includes
check-hsm-versions
check-manpages
check-spelling
check-whitespace

For many of these we have custom scripts that require us to maintain
them as well, despite there being off-the-shelf tooling that allows us
to check and enfore these.

pre-commit standardizes the execution of these checkers, running
them against the proposed changes in a PR. It allows for checking and
fixing as separate steps, so CI can just check, and show the diff of
changes that would have been applied.

The --from-ref and --to-ref params help us to limit the scope of
the checks to the proposed changes, allowing us to incrementally adopt
these standards as we visit the files.

If you adopt a new file, i.e., modify a file that has not been checked
before, you might want to just quickly run pre-commit run --from-ref=master --to-ref=HEAD after the first modification, and fix
up any lints or failing checks before moving on to actually
implementing your changes. This keeps the lint cleanup separate from
logic changes that are to be reviewed.

By limiting the commits that we look at to the changes since `master` we can incrementally pull files under the coverage of these lints and checks. Changelog-None: Not applicable, this is DX

s373nZ · 2024-11-29T20:30:19Z

I see flake8 checks in the Makefile as well. Is the intention here as well to replace flake8 with ruff?

cdecker · 2024-11-30T13:37:42Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

s373nZ · 2024-11-30T13:51:50Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

I should have phrased my comment more clearly. I see the Makefile is utilizing flake8 to run checks on the Python code using the check-python-flake8 target. My question is if the intention is to replace flake8 checks with ruff or if we want to add the flake8 existing checks in an additional hook.

cdecker · 2024-12-08T10:52:04Z

Oh, I see. The intention is to use one l'Inter per language. Otherwise we might get into a territory where they disagree causing unresolvable lint warnings.

Let's remove flake8 altogether.

s373nZ · 2025-03-11T22:47:42Z

For others reviewing this PR, here is another demo PR against the original branch containing some initial precommit experiments: cdecker#7

rustyrussell · 2025-04-01T03:03:34Z

Good start! Let's merge this so we can build on it!

cdecker · 2024-11-28T14:40:55Z

We have quite the variety of different formatters, checkers and linters:

Python formatting, code style and linting
shellcheck run via make
check-amount-access grepping for access to the u64 inside of the amount_msat and amount_sat structs.
JSON-Schema formatting
discouraged-fundctions
check-gen-updated
check-includes
- check-src-includes
- check-hdr-includes
check-hsm-versions
check-manpages
check-spelling
check-whitespace

For many of these we have custom scripts that require us to maintain
them as well, despite there being off-the-shelf tooling that allows us
to check and enfore these.

pre-commit standardizes the execution of these checkers, running
them against the proposed changes in a PR. It allows for checking and
fixing as separate steps, so CI can just check, and show the diff of
changes that would have been applied.

The --from-ref and --to-ref params help us to limit the scope of
the checks to the proposed changes, allowing us to incrementally adopt
these standards as we visit the files.

If you adopt a new file, i.e., modify a file that has not been checked
before, you might want to just quickly run pre-commit run --from-ref=master --to-ref=HEAD after the first modification, and fix
up any lints or failing checks before moving on to actually
implementing your changes. This keeps the lint cleanup separate from
logic changes that are to be reviewed.

By limiting the commits that we look at to the changes since `master` we can incrementally pull files under the coverage of these lints and checks. Changelog-None: Not applicable, this is DX

s373nZ · 2024-11-29T20:30:19Z

I see flake8 checks in the Makefile as well. Is the intention here as well to replace flake8 with ruff?

cdecker · 2024-11-30T13:37:42Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

s373nZ · 2024-11-30T13:51:50Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

I should have phrased my comment more clearly. I see the Makefile is utilizing flake8 to run checks on the Python code using the check-python-flake8 target. My question is if the intention is to replace flake8 checks with ruff or if we want to add the flake8 existing checks in an additional hook.

cdecker · 2024-12-08T10:52:04Z

Oh, I see. The intention is to use one l'Inter per language. Otherwise we might get into a territory where they disagree causing unresolvable lint warnings.

Let's remove flake8 altogether.

s373nZ · 2025-03-11T22:47:42Z

For others reviewing this PR, here is another demo PR against the original branch containing some initial precommit experiments: cdecker#7

rustyrussell · 2025-04-01T03:03:34Z

Good start! Let's merge this so we can build on it!

cdecker · 2024-11-28T14:40:55Z

We have quite the variety of different formatters, checkers and linters:

Python formatting, code style and linting
shellcheck run via make
check-amount-access grepping for access to the u64 inside of the amount_msat and amount_sat structs.
JSON-Schema formatting
discouraged-fundctions
check-gen-updated
check-includes
- check-src-includes
- check-hdr-includes
check-hsm-versions
check-manpages
check-spelling
check-whitespace

For many of these we have custom scripts that require us to maintain
them as well, despite there being off-the-shelf tooling that allows us
to check and enfore these.

pre-commit standardizes the execution of these checkers, running
them against the proposed changes in a PR. It allows for checking and
fixing as separate steps, so CI can just check, and show the diff of
changes that would have been applied.

The --from-ref and --to-ref params help us to limit the scope of
the checks to the proposed changes, allowing us to incrementally adopt
these standards as we visit the files.

If you adopt a new file, i.e., modify a file that has not been checked
before, you might want to just quickly run pre-commit run --from-ref=master --to-ref=HEAD after the first modification, and fix
up any lints or failing checks before moving on to actually
implementing your changes. This keeps the lint cleanup separate from
logic changes that are to be reviewed.

By limiting the commits that we look at to the changes since `master` we can incrementally pull files under the coverage of these lints and checks. Changelog-None: Not applicable, this is DX

s373nZ · 2024-11-29T20:30:19Z

I see flake8 checks in the Makefile as well. Is the intention here as well to replace flake8 with ruff?

cdecker · 2024-11-30T13:37:42Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

s373nZ · 2024-11-30T13:51:50Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

I should have phrased my comment more clearly. I see the Makefile is utilizing flake8 to run checks on the Python code using the check-python-flake8 target. My question is if the intention is to replace flake8 checks with ruff or if we want to add the flake8 existing checks in an additional hook.

cdecker · 2024-12-08T10:52:04Z

Oh, I see. The intention is to use one l'Inter per language. Otherwise we might get into a territory where they disagree causing unresolvable lint warnings.

Let's remove flake8 altogether.

s373nZ · 2025-03-11T22:47:42Z

For others reviewing this PR, here is another demo PR against the original branch containing some initial precommit experiments: cdecker#7

rustyrussell · 2025-04-01T03:03:34Z

Good start! Let's merge this so we can build on it!

cdecker · 2024-11-28T14:40:55Z

We have quite the variety of different formatters, checkers and linters:

Python formatting, code style and linting
shellcheck run via make
check-amount-access grepping for access to the u64 inside of the amount_msat and amount_sat structs.
JSON-Schema formatting
discouraged-fundctions
check-gen-updated
check-includes
- check-src-includes
- check-hdr-includes
check-hsm-versions
check-manpages
check-spelling
check-whitespace

For many of these we have custom scripts that require us to maintain
them as well, despite there being off-the-shelf tooling that allows us
to check and enfore these.

pre-commit standardizes the execution of these checkers, running
them against the proposed changes in a PR. It allows for checking and
fixing as separate steps, so CI can just check, and show the diff of
changes that would have been applied.

The --from-ref and --to-ref params help us to limit the scope of
the checks to the proposed changes, allowing us to incrementally adopt
these standards as we visit the files.

If you adopt a new file, i.e., modify a file that has not been checked
before, you might want to just quickly run pre-commit run --from-ref=master --to-ref=HEAD after the first modification, and fix
up any lints or failing checks before moving on to actually
implementing your changes. This keeps the lint cleanup separate from
logic changes that are to be reviewed.

By limiting the commits that we look at to the changes since `master` we can incrementally pull files under the coverage of these lints and checks. Changelog-None: Not applicable, this is DX

s373nZ · 2024-11-29T20:30:19Z

I see flake8 checks in the Makefile as well. Is the intention here as well to replace flake8 with ruff?

cdecker · 2024-11-30T13:37:42Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

s373nZ · 2024-11-30T13:51:50Z

Whoops? What is flake8 doing with the Makefile? Presumably it fails to parse it as python?

I should have phrased my comment more clearly. I see the Makefile is utilizing flake8 to run checks on the Python code using the check-python-flake8 target. My question is if the intention is to replace flake8 checks with ruff or if we want to add the flake8 existing checks in an additional hook.

cdecker · 2024-12-08T10:52:04Z

Oh, I see. The intention is to use one l'Inter per language. Otherwise we might get into a territory where they disagree causing unresolvable lint warnings.

Let's remove flake8 altogether.

s373nZ · 2025-03-11T22:47:42Z

For others reviewing this PR, here is another demo PR against the original branch containing some initial precommit experiments: cdecker#7

rustyrussell · 2025-04-01T03:03:34Z

Good start! Let's merge this so we can build on it!

chore: Add a simple pre-commit config
333278f
By limiting the commits that we look at to the changes since `master` we can incrementally pull files under the coverage of these lints and checks. Changelog-None: Not applicable, this is DX

cdecker mentioned this pull request Nov 29, 2024
Perform basic conventional checks in Git pre-commit hook #7765
Open
5 tasks

s373nZ mentioned this pull request Nov 29, 2024
dx: Add additional hooks to pre-commit cdecker/lightning#7
Closed

rustyrussell merged commit f38cc0e into ElementsProject:masterApr 1, 2025

s373nZ mentioned this pull request Apr 1, 2025
pre-commit hooks demo #8193
Merged
3 tasks