search

scipy-conference / scipy_proceedings

Tools used to generate the SciPy conference proceedings
Other
228 stars 530 forks source link

feat: Add pre-commit config #1017

Open matthewfeickert opened 3 weeks ago

matthewfeickert commented 3 weeks ago

Resolves #1016

The second commit in this PR is huge, but it is just applying the results of the added pre-commit hooks via

pre-commit run --all-files

as @tkoyama010 has started setting up pre-commit.ci (https://github.com/scipy-conference/scipy_proceedings/issues/1016#issuecomment-2299911858) and pre-commit.ci will run the equivalent of pre-commit run --all-files on each commit. This commit can get ignored automatically from GitHub's git blame on diffs via adding it to a .git-blame-ignore-revs (c.f. https://github.com/matplotlib/matplotlib/blob/1e8ea2f6ac758b331fb8b6838d69822424e86cb4/.git-blame-ignore-revs for an example).

matthewfeickert commented 3 weeks ago

@scipy-conference/proceedings this is ready for review. Let me know if you have any questions. @tkoyama010 a review from you would be welcome also. :)

tkoyama010 commented 3 weeks ago

A markdown formatter should also be added, but the impact is too great and should be discussed carefully.

matthewfeickert commented 3 weeks ago

A markdown formatter should also be added, but the impact is too great and should be discussed carefully.

I like the idea, but as it would be a big diff

Results of applying igorshubovych/markdownlint-cli: ```console markdownlint.............................................................Failed - hook id: markdownlint - exit code: 1 publisher/getting_ready_for_new_year.md:8:1 MD051/link-fragments Link fragments should be valid [Expected: #what-problem-are-we-solving; Actual: #What-problem-are-we-solving] [Context: "[the problem section](#What-problem-are-we-solving)"] publisher/getting_ready_for_new_year.md:10 MD025/single-title/single-h1 Multiple top-level headings in the same document [Context: "# Workflow"] publisher/getting_ready_for_new_year.md:30:81 MD013/line-length Line length [Expected: 80; Actual: 99] publisher/getting_ready_for_new_year.md:32:81 MD013/line-length Line length [Expected: 80; Actual: 88] publisher/getting_ready_for_new_year.md:33:81 MD013/line-length Line length [Expected: 80; Actual: 91] publisher/getting_ready_for_new_year.md:74:49 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: '.'] publisher/getting_ready_for_new_year.md:89:65 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: '.'] publisher/getting_ready_for_new_year.md:94:1 MD051/link-fragments Link fragments should be valid [Expected: #workflow; Actual: #Workflow] [Context: "[](#Workflow)"] publisher/README.md:1:41 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: '.'] publisher/README.md:5:81 MD013/line-length Line length [Expected: 80; Actual: 153] publisher/README.md:7:81 MD013/line-length Line length [Expected: 80; Actual: 91] publisher/README.md:9:81 MD013/line-length Line length [Expected: 80; Actual: 132] publisher/README.md:11:17 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] publisher/README.md:23:81 MD013/line-length Line length [Expected: 80; Actual: 91] publisher/README.md:27:81 MD013/line-length Line length [Expected: 80; Actual: 242] publisher/README.md:31:81 MD013/line-length Line length [Expected: 80; Actual: 125] publisher/README.md:47:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] publisher/README.md:48:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] publisher/README.md:50:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] publisher/README.md:51:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] publisher/README.md:57:81 MD013/line-length Line length [Expected: 80; Actual: 148] publisher/README.md:67:81 MD013/line-length Line length [Expected: 80; Actual: 93] publisher/README.md:83:81 MD013/line-length Line length [Expected: 80; Actual: 91] publisher/README.md:109:1 MD023/heading-start-left Headings must start at the beginning of the line [Context: " ## DOI metadata"] publisher/README.md:116:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] publisher/README.md:117:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] publisher/README.md:118:1 MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 8] publisher/README.md:119:1 MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 8] publisher/README.md:120:1 MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 8] publisher/README.md:124:2 MD034/no-bare-urls Bare URL used [Context: "http://www.issn.org/understand..."] publisher/README.md:136:2 MD034/no-bare-urls Bare URL used [Context: "https://doi.org/10.25080/shinm..."] publisher/README.md:137 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- which resolves the proceedin..."] pull_request_template.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "If you are creating this PR in..."] pull_request_template.md:2:5 MD034/no-bare-urls Bare URL used [Context: "http://procbuild.scipy.org/"] pull_request_template.md:8 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] README.md:9:81 MD013/line-length Line length [Expected: 80; Actual: 116] README.md:12:81 MD013/line-length Line length [Expected: 80; Actual: 129] README.md:17:81 MD013/line-length Line length [Expected: 80; Actual: 133] README.md:42 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- with many people contributin..."] README.md:43:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:44:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:46:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:46:21 MD034/no-bare-urls Bare URL used [Context: "https://github.com/scipy-confe..."] README.md:47:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:47:15 MD034/no-bare-urls Bare URL used [Context: "https://github.com/scipy-confe..."] README.md:50 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- build system:"] README.md:51:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:52:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:53:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:55:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:56:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:57:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:58:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:73:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:74:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:74:81 MD013/line-length Line length [Expected: 80; Actual: 96] README.md:82:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:83:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:86:34 MD034/no-bare-urls Bare URL used [Context: "http://conference.scipy.org/pr..."] README.md:103 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] README.md:106:81 MD013/line-length Line length [Expected: 80; Actual: 164] README.md:109 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Meghann Agarwal (@mepa)"] README.md:119:81 MD013/line-length Line length [Expected: 80; Actual: 115] README.md:138:81 MD013/line-length Line length [Expected: 80; Actual: 271] README.md:169:51 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] README.md:173:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:174:1 MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 8] README.md:175:1 MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 8] README.md:176:1 MD007/ul-indent Unordered list indentation [Expected: 4; Actual: 8] README.md:177:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:178:53 MD034/no-bare-urls Bare URL used [Context: "http://procbuild.scipy.org"] README.md:182:81 MD013/line-length Line length [Expected: 80; Actual: 112] README.md:183:81 MD013/line-length Line length [Expected: 80; Actual: 126] README.md:190:81 MD013/line-length Line length [Expected: 80; Actual: 135] README.md:210 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"] README.md:216 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"] README.md:224:81 MD013/line-length Line length [Expected: 80; Actual: 99] README.md:227:81 MD013/line-length Line length [Expected: 80; Actual: 123] README.md:228:81 MD013/line-length Line length [Expected: 80; Actual: 167] README.md:228:141 MD034/no-bare-urls Bare URL used [Context: "http://procbuild.scipy.org"] README.md:228:100 MD051/link-fragments Link fragments should be valid [Context: "[check your paper](#check-your-paper)"] README.md:228:41 MD051/link-fragments Link fragments should be valid [Context: "[push changes to your PR's branch](#push-your-changes)"] README.md:241:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:242:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:244:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:246:12 MD038/no-space-in-code Spaces inside code span elements [Context: "`git remote -v `"] README.md:247 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```"] README.md:247 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"] README.md:248:7 MD010/no-hard-tabs Hard tabs [Column: 7] README.md:249:7 MD010/no-hard-tabs Hard tabs [Column: 7] README.md:250:9 MD010/no-hard-tabs Hard tabs [Column: 9] README.md:251:9 MD010/no-hard-tabs Hard tabs [Column: 9] README.md:257:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:259:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:268 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"] README.md:276:81 MD013/line-length Line length [Expected: 80; Actual: 96] README.md:280:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:281:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:287:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:311:81 MD013/line-length Line length [Expected: 80; Actual: 116] README.md:345:81 MD013/line-length Line length [Expected: 80; Actual: 174] README.md:347:81 MD013/line-length Line length [Expected: 80; Actual: 96] README.md:381:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:384:81 MD013/line-length Line length [Expected: 80; Actual: 85] README.md:390:81 MD013/line-length Line length [Expected: 80; Actual: 154] README.md:399 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] README.md:402:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1] README.md:403:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1] README.md:406:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1] README.md:407:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1] README.md:410:30 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] README.md:412 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"] README.md:424 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"] README.md:425:81 MD013/line-length Line length [Expected: 80; Actual: 248] README.md:430:62 MD034/no-bare-urls Bare URL used [Context: "http://procbuild.scipy.org"] README.md:448:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:448:81 MD013/line-length Line length [Expected: 80; Actual: 95] README.md:455:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 2] README.md:455 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- The **labels** in question a..."] README.md:456:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:457:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:458:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:460:81 MD013/line-length Line length [Expected: 80; Actual: 134] README.md:470:1 MD007/ul-indent Unordered list indentation [Expected: 2; Actual: 4] README.md:475 MD001/heading-increment Heading levels should only increment by one level at a time [Expected: h3; Actual: h4] README.md:479:81 MD013/line-length Line length [Expected: 80; Actual: 99] README.md:485:81 MD013/line-length Line length [Expected: 80; Actual: 93] README.md:486:81 MD013/line-length Line length [Expected: 80; Actual: 187] README.md:487 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "affiliation if these cannot be..."] README.md:488 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```json"] README.md:505 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```"] README.md:506:1 MD029/ol-prefix Ordered list item prefix [Expected: 1; Actual: 7; Style: 1/1/1] README.md:506 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "7. [Create a PR](#create-a-pap..."] README.md:508:81 MD013/line-length Line length [Expected: 80; Actual: 91] review_criteria.md:27:81 MD013/line-length Line length [Expected: 80; Actual: 115] review_criteria.md:75 MD025/single-title/single-h1 Multiple top-level headings in the same document [Context: "# Suggestions for Writing Grea..."] review_criteria.md:77:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk] review_criteria.md:77:81 MD013/line-length Line length [Expected: 80; Actual: 86] review_criteria.md:79:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk] review_criteria.md:79:81 MD013/line-length Line length [Expected: 80; Actual: 87] review_criteria.md:83:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk] review_criteria.md:84:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk] review_criteria.md:84:81 MD013/line-length Line length [Expected: 80; Actual: 90] review_criteria.md:87:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk] review_criteria.md:88:81 MD013/line-length Line length [Expected: 80; Actual: 86] review_criteria.md:91:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk] review_criteria.md:91:81 MD013/line-length Line length [Expected: 80; Actual: 91] review_criteria.md:92:81 MD013/line-length Line length [Expected: 80; Actual: 92] review_criteria.md:94:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk] review_criteria.md:96:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk] review_criteria.md:96:81 MD013/line-length Line length [Expected: 80; Actual: 88] review_criteria.md:97:81 MD013/line-length Line length [Expected: 80; Actual: 91] review_criteria.md:98:81 MD013/line-length Line length [Expected: 80; Actual: 91] review_criteria.md:101 MD025/single-title/single-h1 Multiple top-level headings in the same document [Context: "# Proceedings Committee"] review_criteria.md:103:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk] review_criteria.md:103:81 MD013/line-length Line length [Expected: 80; Actual: 238] ```

maybe we should do a follow up PR with it and codespell-project/codespell as I think we'd need input from the proceedings team if they would want all those changes.

tkoyama010 commented 3 weeks ago

Closing to try pre-commit.ci

matthewfeickert commented 2 weeks ago

A note here for @scipy-conference/proceedings given Slack discussion. After this PR is merged this should get backported to the 2024 branch so that all the PRs targeting it for the 2024 proceedings will be targeting a branch with a .pre-commit-config.yaml on it for pre-commit.ci to use.

edit: (Though GitHub won't notify new people tagged in an edit, so I've requested review) This should have been @scipy-conference/2024-proceedings. (Maybe the old team should get retired to avoid confusion.)

TahiriNadia commented 2 weeks ago

Hello,

We have a new error: .pre-commit-config.yaml is not recognized as a file. I think it's related to the new test. Tests: image

precommit.ci: image

See PR 925, and we look at the log.

matthewfeickert commented 2 weeks ago

@TahiriNadia c.f. https://github.com/scipy-conference/scipy_proceedings/pull/1017#issuecomment-2305306115

matthewfeickert commented 2 weeks ago

Is there some reason this branch seeks to make so many minor formatting and other changes to so many files other than just adding .pre-commit-config.yaml ?

@cbcunc Please read the PR body where this is explained: https://github.com/scipy-conference/scipy_proceedings/pull/1017#issue-2476970734

cbcunc commented 2 weeks ago

@cbcunc Please read the PR body where this is explained: #1017 (comment)

Does not explain why it needs to run on --all-files.

matthewfeickert commented 2 weeks ago

Does not explain why it needs to run on --all-files.

@scipy-conference/2024-proceedings Okay, let me try to unpack

The second commit in this PR is huge, but it is just applying the results of the added pre-commit hooks via

pre-commit run --all-files

as @tkoyama010 has started setting up pre-commit.ci (https://github.com/scipy-conference/scipy_proceedings/issues/1016#issuecomment-2299911858) and pre-commit.ci will run the equivalent of pre-commit run --all-files on each commit. This commit can get ignored automatically from GitHub's git blame on diffs via adding it to a .git-blame-ignore-revs (c.f. https://github.com/matplotlib/matplotlib/blob/1e8ea2f6ac758b331fb8b6838d69822424e86cb4/.git-blame-ignore-revs for an example).

in much more detail then.

I think that should explain why pre-commit run --all-files was used.

As I also mentioned, I understand that this is a large change, and people generally don't want to have to deal with this noise when using git blame to try to trace the history of changes through a file. This is why .git-blame-ignore-revs exists, as it will be automatically picked up by GitHub and all the commits listed in it will be ignored for git blame reasons. You can manually apply it at the command line with --ignore-revs-file. Many projects do this to ignore big stylistic changes from their git blames, like turning on pre-commit or adding a new linter (like black or ruff). Matplotlib is an example of one of those projects, so I list it here as an example if people want to see how they're using it.

I did not include a .git-blame-ignore-revs in this PR as it requires the commit hash. If this was going to be squashed and merged then the commit hash would change and the file would be wrong. So I would suggest adding a .git-blame-ignore-revs after this PR is in and the commits are known.

matthewfeickert commented 2 weeks ago

A note here for @scipy-conference/proceedings given Slack discussion. After this PR is merged this should get backported to the 2024 branch so that all the PRs targeting it for the 2024 proceedings will be targeting a branch with a .pre-commit-config.yaml on it for pre-commit.ci to use.

Be aware that when this is backported to the 2024 branch there is already a .pre-commit-config.yaml file there from PR https://github.com/scipy-conference/scipy_proceedings/pull/1018 that will get overwritten.

matthewfeickert commented 2 weeks ago

@scipy-conference/2024-proceedings I see from PR https://github.com/scipy-conference/scipy_proceedings/pull/925 that pre-commit.ci has been disabled as it doesn't show up in the checks and when you look at the pre-commit.ci page for the repo (https://results.pre-commit.ci/repo/github/1825629) it was disabled before the last pushes https://github.com/scipy-conference/scipy_proceedings/pull/925#commits-pushed-bba46c3.

Is there still interest in using pre-commit.ci (even if not while the 2024 proceedings are still in process)?

cbcunc commented 2 weeks ago

Is there still interest in using pre-commit.ci (even if not while the 2024 proceedings are still in process)?

Yes, after we're done with the 2024 process.

mepa commented 1 week ago

Is there still interest in using pre-commit.ci (even if not while the 2024 proceedings are still in process)?

In my original comment I should have mentioned incorporating use of pre-commit.ci after the 2024 proceedings are published. Thank you, @matthewfeickert and @tkoyama010, for your work on this.