ci: bring bitcoin’s .py linter into secp256k1 #1278

1. 
   RandomLattice commented at 11:24 am on April 14, 2023: contributor

   Now that #1245 introduced a build-time dependency on python3, we probably want to run bitcoin’s python linter in this repo. @sipa had to disable this in https://github.com/bitcoin/bitcoin/pull/27445 since we didn’t catch linting errors in secp256k1’s CI but only in bitcoin’s CI.

   What would be a good course of action? Some options:

   1. Add a CI job in secp256k1 to do a shallow clone of bitcoin master and run the linter
   2. Copy linter scripts from bitcoin into secp256k1

   No option is perfect: 1. is probably a bit heavy, and 2 will get outdated. I am leaning towards 1. Thoughts?

2. 
   real-or-random commented at 12:29 pm on April 14, 2023: contributor

   To be honest, I lean towards doing nothing.

   For example, I don’t see that #1279 is an obvious improvement in readability. We also don’t (syntax) linting for our C files, and I think our experience with that is rather good. It provides authors with a bit more freedom in making deliberate choices about their code. Of course, that freedom comes with responsibility, but we’re a small project, and we never had real issues with that. Things may be different in Core with many different contributors.

   Moreover, we have more *.py files, they just masquerade as *.sage files. So if we start linting .py, we’d also need to lint *.sage, and that probably requires more effort without clear benefits.

   And I don’t think disabling linting in https://github.com/bitcoin/bitcoin/pull/27445 should be a temporary solution. It’s just wrong for Core to lint vendored code.

3. 
   sipa commented at 12:50 pm on April 14, 2023: contributor

   I think it’s perhaps worth doing a one-shot “make our scripts satisfy some linter complaints”, but not enforce anything in secp256k1’s CI. Our scripts rarely change, and some linter complaints are actually pretty useful advice for making the code more readable (see my comments in #1279). I also think the situation is a bit different for Python than for C (where the Python community has a much more uniform expectation of what a reasonable style is).

   I agree that disabling linting in https://github.com/bitcoin/bitcoin/pull/27445 should not be temporary. I just marked the PRs there are WIP because they’re likely not the right approach to doing so (we’d need to investigate if they’re not disabling other things than the secp256k1 subtree, e.g.).

4. 
   RandomLattice commented at 1:18 pm on April 14, 2023: contributor

   Cool, I’m glad I asked before. Let’s not do anything in CI, and focus on a one-off in #1279.
5. 
   ekzyis commented at 10:31 am on May 5, 2023: none

   This can be closed now that #1279 is merged, right?
6. real-or-random closed this on May 5, 2023

   ---

Contributors
RandomLattice real-or-random sipa ekzyis