Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Meta: Add trailing-whitespace and end-of-file-fixer to pre-commit config #4067

Open
lysnikolaou opened this issue Oct 18, 2024 · 8 comments
Open
Labels
meta Related to the repo itself and its processes

Comments

@lysnikolaou
Copy link
Contributor

I was trying to remove all trailing whitespace from PEP 750 and noticed that we currently do not have trailing-whitespace and end-of-file-fixer in our pre-commit config. I think it makes sense to include these. From the pre-commit-hooks README:

end-of-file-fixer: Makes sure files end in a newline and only a newline.

trailing-whitespace
Trims trailing whitespace.

  • To preserve Markdown hard linebreaks
    use args: [--markdown-linebreak-ext=md] (or other extensions used
    by your markdownfiles). If for some reason you want to treat all files
    as markdown, use --markdown-linebreak-ext=*.
  • By default, this hook trims all whitespace from the ends of lines.
    To specify a custom set of characters to trim instead, use args: [--chars,"<chars to trim>"].

Do people have objections?

@hugovk
Copy link
Member

hugovk commented Oct 18, 2024

Fine by me. It will affect 243 files, better to do them in one go to avoid over-spamming.

Are there any other similar rules we should add at the same time?

243 files
#       modified:  [1] .github/PULL_REQUEST_TEMPLATE/Mark a PEP Final.md
#       modified:  [2] .pre-commit-config.yaml
#       modified:  [3] pep_sphinx_extensions/tests/peps/pep-9000.rst
#       modified:  [4] peps/pep-0008.rst
#       modified:  [5] peps/pep-0200.rst
#       modified:  [6] peps/pep-0230.rst
#       modified:  [7] peps/pep-0247.rst
#       modified:  [8] peps/pep-0256.rst
#       modified:  [9] peps/pep-0258.rst
#       modified: [10] peps/pep-0265.rst
#       modified: [11] peps/pep-0272.rst
#       modified: [12] peps/pep-0282.rst
#       modified: [13] peps/pep-0287.rst
#       modified: [14] peps/pep-0290.rst
#       modified: [15] peps/pep-0301.rst
#       modified: [16] peps/pep-0302.rst
#       modified: [17] peps/pep-0304.rst
#       modified: [18] peps/pep-0305.rst
#       modified: [19] peps/pep-0307.rst
#       modified: [20] peps/pep-0309.rst
#       modified: [21] peps/pep-0316.rst
#       modified: [22] peps/pep-0317.rst
#       modified: [23] peps/pep-0318.rst
#       modified: [24] peps/pep-0321.rst
#       modified: [25] peps/pep-0327.rst
#       modified: [26] peps/pep-0328.rst
#       modified: [27] peps/pep-0332.rst
#       modified: [28] peps/pep-0333.rst
#       modified: [29] peps/pep-0334.rst
#       modified: [30] peps/pep-0335.rst
#       modified: [31] peps/pep-0339.rst
#       modified: [32] peps/pep-0342.rst
#       modified: [33] peps/pep-0345.rst
#       modified: [34] peps/pep-0346.rst
#       modified: [35] peps/pep-0348.rst
#       modified: [36] peps/pep-0351.rst
#       modified: [37] peps/pep-0352.rst
#       modified: [38] peps/pep-0353.rst
#       modified: [39] peps/pep-0354.rst
#       modified: [40] peps/pep-0359.rst
#       modified: [41] peps/pep-0360.rst
#       modified: [42] peps/pep-0362.rst
#       modified: [43] peps/pep-0364.rst
#       modified: [44] peps/pep-0365.rst
#       modified: [45] peps/pep-0367.rst
#       modified: [46] peps/pep-0368.rst
#       modified: [47] peps/pep-0369.rst
#       modified: [48] peps/pep-0373.rst
#       modified: [49] peps/pep-0374.rst
#       modified: [50] peps/pep-0375.rst
#       modified: [51] peps/pep-0376.rst
#       modified: [52] peps/pep-0378.rst
#       modified: [53] peps/pep-0380.rst
#       modified: [54] peps/pep-0385.rst
#       modified: [55] peps/pep-0386.rst
#       modified: [56] peps/pep-0389.rst
#       modified: [57] peps/pep-0390.rst
#       modified: [58] peps/pep-0391.rst
#       modified: [59] peps/pep-0392.rst
#       modified: [60] peps/pep-0396.rst
#       modified: [61] peps/pep-0398.rst
#       modified: [62] peps/pep-0400.rst
#       modified: [63] peps/pep-0404.rst
#       modified: [64] peps/pep-0405.rst
#       modified: [65] peps/pep-0409.rst
#       modified: [66] peps/pep-0410.rst
#       modified: [67] peps/pep-0412.rst
#       modified: [68] peps/pep-0414.rst
#       modified: [69] peps/pep-0415.rst
#       modified: [70] peps/pep-0416.rst
#       modified: [71] peps/pep-0417.rst
#       modified: [72] peps/pep-0418/bench_time.c
#       modified: [73] peps/pep-0418/clock_resolution.py
#       modified: [74] peps/pep-0420.rst
#       modified: [75] peps/pep-0421.rst
#       modified: [76] peps/pep-0423.rst
#       modified: [77] peps/pep-0424.rst
#       modified: [78] peps/pep-0426/pepsort.py
#       modified: [79] peps/pep-0429.rst
#       modified: [80] peps/pep-0433.rst
#       modified: [81] peps/pep-0434.rst
#       modified: [82] peps/pep-0436.rst
#       modified: [83] peps/pep-0437.rst
#       modified: [84] peps/pep-0438.rst
#       modified: [85] peps/pep-0439.rst
#       modified: [86] peps/pep-0443.rst
#       modified: [87] peps/pep-0444.rst
#       modified: [88] peps/pep-0445.rst
#       modified: [89] peps/pep-0446.rst
#       modified: [90] peps/pep-0446/test_cloexec.py
#       modified: [91] peps/pep-0449.rst
#       modified: [92] peps/pep-0451.rst
#       modified: [93] peps/pep-0454.rst
#       modified: [94] peps/pep-0455.rst
#       modified: [95] peps/pep-0456.rst
#       modified: [96] peps/pep-0457.rst
#       modified: [97] peps/pep-0460.rst
#       modified: [98] peps/pep-0463.rst
#       modified: [99] peps/pep-0464.rst
#       modified: [100] peps/pep-0466.rst
#       modified: [101] peps/pep-0468.rst
#       modified: [102] peps/pep-0470.rst
#       modified: [103] peps/pep-0471.rst
#       modified: [104] peps/pep-0472.rst
#       modified: [105] peps/pep-0473.rst
#       modified: [106] peps/pep-0475.rst
#       modified: [107] peps/pep-0478.rst
#       modified: [108] peps/pep-0479.rst
#       modified: [109] peps/pep-0483.rst
#       modified: [110] peps/pep-0484.rst
#       modified: [111] peps/pep-0486.rst
#       modified: [112] peps/pep-0490.rst
#       modified: [113] peps/pep-0494.rst
#       modified: [114] peps/pep-0495-gap.svg
#       modified: [115] peps/pep-0498.rst
#       modified: [116] peps/pep-0503.rst
#       modified: [117] peps/pep-0504.rst
#       modified: [118] peps/pep-0506.rst
#       modified: [119] peps/pep-0508.rst
#       modified: [120] peps/pep-0515.rst
#       modified: [121] peps/pep-0517.rst
#       modified: [122] peps/pep-0519.rst
#       modified: [123] peps/pep-0520.rst
#       modified: [124] peps/pep-0521.rst
#       modified: [125] peps/pep-0524.rst
#       modified: [126] peps/pep-0527.rst
#       modified: [127] peps/pep-0532/circuit-breaking-protocol.svg
#       modified: [128] peps/pep-0536.rst
#       modified: [129] peps/pep-0537.rst
#       modified: [130] peps/pep-0541.rst
#       modified: [131] peps/pep-0543.rst
#       modified: [132] peps/pep-0544.rst
#       modified: [133] peps/pep-0546.rst
#       modified: [134] peps/pep-0547.rst
#       modified: [135] peps/pep-0552.rst
#       modified: [136] peps/pep-0556.rst
#       modified: [137] peps/pep-0559.rst
#       modified: [138] peps/pep-0561.rst
#       modified: [139] peps/pep-0562.rst
#       modified: [140] peps/pep-0563.rst
#       modified: [141] peps/pep-0566.rst
#       modified: [142] peps/pep-0575.rst
#       modified: [143] peps/pep-0579.rst
#       modified: [144] peps/pep-0580.rst
#       modified: [145] peps/pep-0582.rst
#       modified: [146] peps/pep-0586.rst
#       modified: [147] peps/pep-0606.rst
#       modified: [148] peps/pep-0607.rst
#       modified: [149] peps/pep-0608.rst
#       modified: [150] peps/pep-0610.rst
#       modified: [151] peps/pep-0611.rst
#       modified: [152] peps/pep-0615.rst
#       modified: [153] peps/pep-0616.rst
#       modified: [154] peps/pep-0617.rst
#       modified: [155] peps/pep-0625.rst
#       modified: [156] peps/pep-0626.rst
#       modified: [157] peps/pep-0629.rst
#       modified: [158] peps/pep-0632.rst
#       modified: [159] peps/pep-0636.rst
#       modified: [160] peps/pep-0637.rst
#       modified: [161] peps/pep-0639/appendix-rejected-ideas.rst
#       modified: [162] peps/pep-0641.rst
#       modified: [163] peps/pep-0643.rst
#       modified: [164] peps/pep-0645.rst
#       modified: [165] peps/pep-0646.rst
#       modified: [166] peps/pep-0651.rst
#       modified: [167] peps/pep-0652.rst
#       modified: [168] peps/pep-0653.rst
#       modified: [169] peps/pep-0657.rst
#       modified: [170] peps/pep-0659.rst
#       modified: [171] peps/pep-0660.rst
#       modified: [172] peps/pep-0662.rst
#       modified: [173] peps/pep-0669.rst
#       modified: [174] peps/pep-0672.rst
#       modified: [175] peps/pep-0676.rst
#       modified: [176] peps/pep-0679.rst
#       modified: [177] peps/pep-0681.rst
#       modified: [178] peps/pep-0682.rst
#       modified: [179] peps/pep-0691.rst
#       modified: [180] peps/pep-0692.rst
#       modified: [181] peps/pep-0693.rst
#       modified: [182] peps/pep-0694.rst
#       modified: [183] peps/pep-0695.rst
#       modified: [184] peps/pep-0699.rst
#       modified: [185] peps/pep-0703.rst
#       modified: [186] peps/pep-0705.rst
#       modified: [187] peps/pep-0706.rst
#       modified: [188] peps/pep-0713.rst
#       modified: [189] peps/pep-0719.rst
#       modified: [190] peps/pep-0722.rst
#       modified: [191] peps/pep-0724.rst
#       modified: [192] peps/pep-0725.rst
#       modified: [193] peps/pep-0727.rst
#       modified: [194] peps/pep-0728.rst
#       modified: [195] peps/pep-0732-concentric.drawio.svg
#       modified: [196] peps/pep-0732.rst
#       modified: [197] peps/pep-0736.rst
#       modified: [198] peps/pep-0744.rst
#       modified: [199] peps/pep-0746.rst
#       modified: [200] peps/pep-0747.rst
#       modified: [201] peps/pep-0749.rst
#       modified: [202] peps/pep-0754.rst
#       modified: [203] peps/pep-0756.rst
#       modified: [204] peps/pep-0758.rst
#       modified: [205] peps/pep-0760.rst
#       modified: [206] peps/pep-0762.rst
#       modified: [207] peps/pep-0777.rst
#       modified: [208] peps/pep-3000.rst
#       modified: [209] peps/pep-3002.rst
#       modified: [210] peps/pep-3104.rst
#       modified: [211] peps/pep-3105.rst
#       modified: [212] peps/pep-3107.rst
#       modified: [213] peps/pep-3108.rst
#       modified: [214] peps/pep-3109.rst
#       modified: [215] peps/pep-3110.rst
#       modified: [216] peps/pep-3111.rst
#       modified: [217] peps/pep-3113.rst
#       modified: [218] peps/pep-3116.rst
#       modified: [219] peps/pep-3117.rst
#       modified: [220] peps/pep-3118.rst
#       modified: [221] peps/pep-3120.rst
#       modified: [222] peps/pep-3121.rst
#       modified: [223] peps/pep-3123.rst
#       modified: [224] peps/pep-3124.rst
#       modified: [225] peps/pep-3125.rst
#       modified: [226] peps/pep-3126.rst
#       modified: [227] peps/pep-3127.rst
#       modified: [228] peps/pep-3128.rst
#       modified: [229] peps/pep-3129.rst
#       modified: [230] peps/pep-3131.rst
#       modified: [231] peps/pep-3132.rst
#       modified: [232] peps/pep-3133.rst
#       modified: [233] peps/pep-3136.rst
#       modified: [234] peps/pep-3137.rst
#       modified: [235] peps/pep-3138.rst
#       modified: [236] peps/pep-3139.rst
#       modified: [237] peps/pep-3141.rst
#       modified: [238] peps/pep-3146.rst
#       modified: [239] peps/pep-3148.rst
#       modified: [240] peps/pep-3149.rst
#       modified: [241] peps/pep-3152.rst
#       modified: [242] peps/pep-3153.rst
#       modified: [243] peps/pep-3156.rst

@hugovk hugovk added the meta Related to the repo itself and its processes label Oct 18, 2024
@lysnikolaou
Copy link
Contributor Author

lysnikolaou commented Oct 22, 2024

I tried opening a PR this morning and 54 reviewers were added because of CODEOWNERS. Do we think that's acceptable?

@AA-Turner
Copy link
Member

Can we exclude those files from the hook? Then only new PEPs will be affected.

@lysnikolaou
Copy link
Contributor Author

Not sure how to do that without having to enumerate all of the files currently in the repo. If anyone has any ideas for this, I'd be happy to do it.

@hugovk
Copy link
Member

hugovk commented Oct 22, 2024

I tried opening a PR for this this morning and 54 reviewers were added because of CODEOWNERS. Do we think that's acceptable?

For a one-off with a quick merge, I think it's okay. We get worse on mistake PRs quite regularly!

Can we exclude those files from the hook? Then only new PEPs will be affected.

The exclude is a regex: https://pre-commit.com/#pre-commit-configyaml---hooks

Exclude something like ^peps/pep-[03][0-6].* and take the hit on the 7xx series?

@lysnikolaou
Copy link
Contributor Author

The exclude is a regex: pre-commit.com#pre-commit-configyaml---hooks

Exclude something like ^peps/pep-[03][0-6].* and take the hit on the 7xx series?

Right! This takes it down to 19 requested reviews. Should we go ahead and do that?

@AA-Turner
Copy link
Member

Oops sorry, missed this - I opened #4078 as an illustration of what I meant but it is pretty much just replicating Hugo's point. An alternative would be to enumerate every number, which wouldn't be too painful.

@JelleZijlstra
Copy link
Member

For a one-off with a quick merge, I think it's okay. We get worse on mistake PRs quite regularly!

A workaround we sometimes use is to keep the PR in draft until it's ready, then quickly take it out of draft and merge it. Everyone will still get a notification, but they'll see the PR is already merged and they won't have to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to the repo itself and its processes
Projects
None yet
Development

No branches or pull requests

5 participants
@JelleZijlstra @hugovk @AA-Turner @lysnikolaou and others