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

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

Closed
RandomLattice opened this issue Apr 14, 2023 · 4 comments
Closed

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

RandomLattice opened this issue Apr 14, 2023 · 4 comments

Comments

@RandomLattice
Copy link
Contributor

RandomLattice commented Apr 14, 2023

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 bitcoin/bitcoin#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?

@real-or-random
Copy link
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 bitcoin/bitcoin#27445 should be a temporary solution. It's just wrong for Core to lint vendored code.

@sipa
Copy link
Contributor

sipa commented Apr 14, 2023

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 bitcoin/bitcoin#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.).

@RandomLattice
Copy link
Contributor Author

Cool, I'm glad I asked before. Let's not do anything in CI, and focus on a one-off in #1279.

@ekzyis
Copy link

ekzyis commented May 5, 2023

This can be closed now that #1279 is merged, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants