Add pre-commit config with pre-commit-hooks #3283

weiji14 · 2024-06-04T05:28:20Z

Description of proposed changes

Add pre-commit hooks to enforce certain rules on all files in the repo.

The following rules are applied:

check-added-large-files
check-yaml
end-of-file-fixer
trailing-whitespace
forbid-crlf
remove-crlf
chmod

Note that pre-commit run --all-files has been added to the Makefile, so running make format locally or /format on a GitHub PR will apply the formatting rules.

TODO:

Add a .pre-commit-config.yaml file for standard pre-commit hooks
~~Configure pre-commit-ci GitHub App~~ not using, doing pre-commit manually ourselves

References:

https://pre-commit.com/index.html#quick-start

Fixes #1593

Reminders

Run make format and make check to make sure the code follows the style guide.
Add tests for new features or tests that would have caught the bug that you're fixing.
Add new public functions/methods/classes to doc/api/index.rst.
Write detailed docstrings for all functions/methods.
If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
If adding new functionality, add an example to docstrings or tutorials.
Use underscores (not hyphens) in names of Python files and directories.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash command is:

/format: automatically format and lint the code

Adding a .pre-commit-config.yaml file with some pre-commit hooks (check-added-large-files, check-yaml, end-of-file-fixer, trailing-whitespace). Also added pre-commit.ci config with autofix_prs=false, and autoupdate_schedule=quarterly.

Copied from https://results.pre-commit.ci/repo/github/85352251

weiji14 · 2024-06-04T05:41:08Z

Results from first pre-commit.ci run at https://results.pre-commit.ci/run/github/85352251/1717479459.EAFPxChRS_aFLgAcskCvfQ:

check for added large files..............................................Passed
check yaml...............................................................Passed
fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing doc/_templates/autosummary/class.rst
Fixing doc/_templates/autosummary/exception.rst
Fixing pygmt/tests/data/contours.txt
Fixing doc/_templates/autosummary/function.rst
Fixing doc/team.md
Fixing CODE_OF_CONDUCT.md

trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing examples/projections/README.txt
Fixing doc/changes.md

Do we want to try applying the autofixes directly in this PR?

seisman · 2024-06-04T05:44:21Z

Yes to make sure that autofix works as expected.

Xref https://pre-commit.ci/#configuration-autofix_prs

weiji14 · 2024-06-04T05:46:24Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

weiji14 · 2024-06-04T05:56:35Z

.pre-commit-config.yaml

+ - id: check-added-large-files
+ - id: check-yaml
+ - id: end-of-file-fixer
+ - id: trailing-whitespace


Can also add these hooks from https://github.com/Lucas-C/pre-commit-hooks that handle LF file endings and 644 permissions? (Then we can remove these lines from style_checks.yml).

Suggested change

- id: trailing-whitespace

- id: trailing-whitespace

- repo: https://github.com/Lucas-C/pre-commit-hooks

rev: v1.5.5

hooks:

- id: forbid-crlf

- id: remove-crlf

- id: chmod

args: ['644']

Looks good.

Hmm, not sure why, but pre-commit.ci seeems to think our files have 664 permission instead of 644? See logs at https://results.pre-commit.ci/run/github/85352251/1717481309.E-FyucnQQ_CvuFT06LGuHg

I've added back the GitHub Action workflows for 644 permission checks/formatting for now at commit 5a01ad6.

Since we're not using pre-commit.ci anymore but just GitHub Actions, I've reinstated the 644 pre-commit hook in the YAML file. Seems to be working at https://github.com/GenericMappingTools/pygmt/actions/runs/9458943127/job/26055263366?pr=3283#step:5:31:

[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks. [INFO] Initializing environment for https://github.com/Lucas-C/pre-commit-hooks. [INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks. [INFO] Once installed this environment will be reused. [INFO] This may take a few minutes... [INFO] Installing environment for https://github.com/Lucas-C/pre-commit-hooks. [INFO] Once installed this environment will be reused. [INFO] This may take a few minutes... check for added large files..............................................Passed check yaml...............................................................Passed fix end of files.........................................................Passed trim trailing whitespace.................................................Passed CRLF end-lines checker...................................................Passed CRLF end-lines remover...................................................Passed Set file permissions.....................................................Failed - hook id: chmod - exit code: 1 - files were modified by this hook Fixing file permissions on requirements.txt: 0o755 -> 0o644 Error: Process completed with exit code 1.

seisman · 2024-06-04T06:00:30Z

Don't forget to add the .pre-commit-config.yaml file to https://github.com/GenericMappingTools/pygmt/blob/main/MANIFEST.in.

Enforcing LF line endings and 644 permissions via pre-commit hooks!

Partially revert ce6d457 to keep the 644 permission checks and format commands.

weiji14 · 2024-06-04T06:30:43Z

doc/contributing.md

+Even better, you can just write `/format` and/or `pre-commit.ci autofix` in the first
+line of any comment in a pull request to lint the code automatically.


Debating on whether to just add pre-commit run --all-files to the Makefile, or have it listed separately. Main consideration is that Windows users won't have make installed by default, so they would only be able to run pre-commit run --all-files.

make is added as a development dependency in environment.yml, so even Windows developers have make installed.

Oh yes, forgot that it is possible to conda install make now! I'll add it to the Makefile then.

seisman · 2024-06-05T08:39:53Z

Also need to mention pre-commit.ci autofix in the PR template.

README.md

seisman · 2024-06-05T08:39:02Z

doc/contributing.md

+Even better, you can just write `/format` and/or `pre-commit.ci autofix` in the first
+line of any comment in a pull request to lint the code automatically.


make is added as a development dependency in environment.yml, so even Windows developers have make installed.

weiji14

Also need to mention pre-commit.ci autofix in the PR template.

Done at b3eee6f. But I'm also wondering if this is redundant, because I've added pre-commit run --all-files to the make format command, so typing /format will also run pre-commit now 🙂

README.md

seisman · 2024-06-09T08:15:29Z

But I'm also wondering if this is redundant, because I've added pre-commit run --all-files to the make format command, so typing /format will also run pre-commit now

Adding pre-commit run --all-files to make format means that users/maintainers must set up pre-commit locally (although they just need to run pre-commit install once).

What about adding pre-commit run --all-files to https://github.com/GenericMappingTools/pygmt/blob/main/.github/workflows/format-command.yml, so that pre-commit is run when /format is executed?

weiji14 · 2024-06-09T21:21:20Z

But I'm also wondering if this is redundant, because I've added pre-commit run --all-files to the make format command, so typing /format will also run pre-commit now

Adding pre-commit run --all-files to make format means that users/maintainers must set up pre-commit locally (although they just need to run pre-commit install once).

As long as they have pre-commit installed, they don't need to run pre-commit install because make format and pre-commit run --all-files will automatically set up pre-commit on the first run. E.g.:

$ pre-commit clean  # uninstall/clean out pre-commit files
$ make format
ruff check --fix --exit-zero pygmt doc/conf.py examples
All checks passed!
ruff format pygmt doc/conf.py examples
301 files left unchanged
pre-commit run --all-files
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for https://github.com/Lucas-C/pre-commit-hooks.
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/Lucas-C/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
check for added large files..............................................Passed
check yaml...............................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
CRLF end-lines checker...................................................Passed
CRLF end-lines remover...................................................Passed

What about adding pre-commit run --all-files to https://github.com/GenericMappingTools/pygmt/blob/main/.github/workflows/format-command.yml, so that pre-commit is run when /format is executed?

Depends on whether we want pre-commit in the Makefile or not. I'm actually thinking if we want to install ruff as a pre-commit hook (in a separate PR), and then make format would just be an alias for pre-commit run --all-files.

seisman · 2024-06-10T02:50:37Z

As long as they have pre-commit installed, they don't need to run pre-commit install because make format and pre-commit run --all-files will automatically set up pre-commit on the first run.

Good to know. Then we don't need the pre-commit.ci service anymore, right?

weiji14 · 2024-06-10T03:01:25Z

As long as they have pre-commit installed, they don't need to run pre-commit install because make format and pre-commit run --all-files will automatically set up pre-commit on the first run.

Good to know. Then we don't need the pre-commit.ci service anymore, right?

Yes, pre-commit.ci isn't absolutely necessary since we can run it via style_checks.yml. Only other reason to use pre-commit.ci is to have the quarterly updates for the pre-commit hook versions. It doesn't seem like dependabot supports updating .pre-commit-config.yaml yet according to dependabot/dependabot-core#1524.

seisman · 2024-06-10T15:41:22Z

I'm OK with either keeping pre-commit.ci or not.

.github/PULL_REQUEST_TEMPLATE.md

.github/workflows/style_checks.yaml

.github/workflows/format-command.yml

README.md

doc/contributing.md

This reverts commit 0d17730.

This reverts commit 8176a18.

This reverts commit b3eee6f.

Co-Authored-By: Dongdong Tian <[email protected]>

weiji14 · 2024-06-11T02:57:10Z

/format

weiji14 · 2024-06-11T03:03:40Z

Ok, I've removed the pre-commit.ci GitHub App installation from the repo. Let me try the 644 permissions check again to see if it works on GitHub Actions.

Edit: Yep, 644 permission check seems to be ok, see note at #3283 (comment)

This reverts commit 5a01ad6.

This reverts commit 10ccc9b.

seisman

Looks great.

Add pre-commit config with pre-commit-hooks

93dc844

Adding a .pre-commit-config.yaml file with some pre-commit hooks (check-added-large-files, check-yaml, end-of-file-fixer, trailing-whitespace). Also added pre-commit.ci config with autofix_prs=false, and autoupdate_schedule=quarterly.

weiji14 self-assigned this Jun 4, 2024

Add pre-commit CI badge to main README.md

0d17730

Copied from https://results.pre-commit.ci/repo/github/85352251

Mention pre-commit autofix command in doc/contributing.md

8176a18

Xref https://pre-commit.ci/#configuration-autofix_prs

[pre-commit.ci] auto fixes from pre-commit.com hooks

4ae3ba5

for more information, see https://pre-commit.ci

weiji14 commented Jun 4, 2024

View reviewed changes

weiji14 added 3 commits June 4, 2024 18:02

Add .pre-commit-config.yaml to MAINIFEST.in

c23f776

Add forbid-crlf, remove-crlf and chmod hooks, remove from GitHub Actions

ce6d457

Enforcing LF line endings and 644 permissions via pre-commit hooks!

Remove chmod pre-commit hook and use GitHub Action method again

5a01ad6

Partially revert ce6d457 to keep the 644 permission checks and format commands.

weiji14 commented Jun 4, 2024

View reviewed changes

seisman reviewed Jun 5, 2024

View reviewed changes

seisman added this to the 0.13.0 milestone Jun 5, 2024

seisman added the maintenance Boring but important stuff for the core devs label Jun 5, 2024

weiji14 added 2 commits June 9, 2024 18:10

Add pre-commit run --all-files to make format

a444d33

Mention pre-commit autofix command in Pull Request template

b3eee6f

weiji14 commented Jun 9, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

seisman reviewed Jun 11, 2024

View reviewed changes

weiji14 added 3 commits June 11, 2024 14:43

Revert "Add pre-commit CI badge to main README.md"

d937cac

This reverts commit 0d17730.

Revert "Mention pre-commit autofix command in doc/contributing.md"

0f09fb6

This reverts commit 8176a18.

Revert "Mention pre-commit autofix command in Pull Request template"

386ad8e

This reverts commit b3eee6f.

Add pre-commit to Style Checks and format-command workflows

25652fc

Co-Authored-By: Dongdong Tian <[email protected]>

weiji14 added 2 commits June 11, 2024 14:58

[ci skip] Fix end-of-line in style_checks.yaml

f505a99

Remove pre-commit.ci config from .pre-commit-config.yaml

7b67296

weiji14 marked this pull request as ready for review June 11, 2024 03:00

weiji14 added 5 commits June 11, 2024 15:04

Revert "Remove chmod pre-commit hook and use GitHub Action method again"

533dc4f

This reverts commit 5a01ad6.

Temporarily change requirements.txt permissions to 744

10ccc9b

Revert "Temporarily change requirements.txt permissions to 744"

70414ac

This reverts commit 10ccc9b.

Update note on style_checks.yaml to mention ruff+pre-commit is ran

c0e6930

Mention that pre-commit is used to enforce LF ending and 644 permissions

b606f7f

seisman approved these changes Jun 11, 2024

View reviewed changes

weiji14 merged commit b5242be into main Jun 11, 2024
18 checks passed

weiji14 deleted the ci/pre-commit branch June 11, 2024 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pre-commit config with pre-commit-hooks #3283

Add pre-commit config with pre-commit-hooks #3283

weiji14 commented Jun 4, 2024 •

edited

Loading

weiji14 commented Jun 4, 2024

seisman commented Jun 4, 2024

weiji14 commented Jun 4, 2024

weiji14 Jun 4, 2024 •

edited

Loading

seisman Jun 4, 2024

weiji14 Jun 4, 2024 •

edited

Loading

weiji14 Jun 4, 2024

weiji14 Jun 11, 2024

seisman commented Jun 4, 2024

weiji14 Jun 4, 2024 •

edited

Loading

seisman Jun 5, 2024

weiji14 Jun 9, 2024

seisman commented Jun 5, 2024

seisman Jun 5, 2024

weiji14 left a comment

seisman commented Jun 9, 2024

weiji14 commented Jun 9, 2024

seisman commented Jun 10, 2024

weiji14 commented Jun 10, 2024

seisman commented Jun 10, 2024

weiji14 commented Jun 11, 2024

weiji14 commented Jun 11, 2024 •

edited

Loading

seisman left a comment

- - id: trailing-whitespace
+ - id: trailing-whitespace
+- repo: https://github.com/Lucas-C/pre-commit-hooks
+ rev: v1.5.5
+ hooks:
+ - id: forbid-crlf
+ - id: remove-crlf
+ - id: chmod
+ args: ['644']

		Even better, you can just write `/format` and/or `pre-commit.ci autofix` in the first
		line of any comment in a pull request to lint the code automatically.

Add pre-commit config with pre-commit-hooks #3283

Add pre-commit config with pre-commit-hooks #3283

Conversation

weiji14 commented Jun 4, 2024 • edited Loading

weiji14 commented Jun 4, 2024

seisman commented Jun 4, 2024

weiji14 commented Jun 4, 2024

weiji14 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

seisman Jun 4, 2024

Choose a reason for hiding this comment

weiji14 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

weiji14 Jun 4, 2024

Choose a reason for hiding this comment

weiji14 Jun 11, 2024

Choose a reason for hiding this comment

seisman commented Jun 4, 2024

weiji14 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

seisman Jun 5, 2024

Choose a reason for hiding this comment

weiji14 Jun 9, 2024

Choose a reason for hiding this comment

seisman commented Jun 5, 2024

seisman Jun 5, 2024

Choose a reason for hiding this comment

weiji14 left a comment

Choose a reason for hiding this comment

seisman commented Jun 9, 2024

weiji14 commented Jun 9, 2024

seisman commented Jun 10, 2024

weiji14 commented Jun 10, 2024

seisman commented Jun 10, 2024

weiji14 commented Jun 11, 2024

weiji14 commented Jun 11, 2024 • edited Loading

seisman left a comment

Choose a reason for hiding this comment

weiji14 commented Jun 4, 2024 •

edited

Loading

weiji14 Jun 4, 2024 •

edited

Loading

weiji14 Jun 4, 2024 •

edited

Loading

weiji14 Jun 4, 2024 •

edited

Loading

weiji14 commented Jun 11, 2024 •

edited

Loading