Skip to content

Fix live test for Kyoto in python#697

Merged
thunderbiscuit merged 2 commits intobitcoindevkit:masterfrom
rustaceanrob:python-test-kyoto-3-12
Mar 13, 2025
Merged

Fix live test for Kyoto in python#697
thunderbiscuit merged 2 commits intobitcoindevkit:masterfrom
rustaceanrob:python-test-kyoto-3-12

Conversation

@rustaceanrob
Copy link
Copy Markdown
Collaborator

Description

I recently installed mypy and ruff for python linting and got a bunch of lints on the live Kyoto test:

  1. Named imports are preferred over *
  2. The update method returns an optional
  3. I never actually defined the log_task variable somehow, which is a log loop that prints to the console.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@rustaceanrob rustaceanrob changed the title Fix live test for kyoto module Fix live test for Kyoto in python Mar 13, 2025
@rustaceanrob rustaceanrob force-pushed the python-test-kyoto-3-12 branch from adf1bee to f76840f Compare March 13, 2025 16:31
@thunderbiscuit
Copy link
Copy Markdown
Member

I like the idea of using linters, and we probably should have been using them for a while.

Would you mind adding a commit that passes all python tests through the linter? I'm not super familiar with how the linters work on Python (or the different options, but I'll trust your choice of ruff), but I also assume there might be files to commit that define rules and whatnot? Feel free to commit those as well so that other users can run the linter too.

@rustaceanrob
Copy link
Copy Markdown
Collaborator Author

rustaceanrob commented Mar 13, 2025

Would you mind adding a commit that passes all python tests through the linter?

Interesting, hadn't thought of adding them to CI. I should've been more specific, but I installed these as neovim plug-ins, so I will have to look into a ruff CLI tool or something like that. Do you mind if we punt that to another PR and keep this one as a test fix? The log function introduced in this could help debugging a failed CI

edit: Nevermind I was able to add a lint step to the CI easily enough.

@rustaceanrob rustaceanrob force-pushed the python-test-kyoto-3-12 branch 6 times, most recently from badf81e to d39d27d Compare March 13, 2025 19:40
@rustaceanrob rustaceanrob force-pushed the python-test-kyoto-3-12 branch from d39d27d to af17607 Compare March 13, 2025 19:58
@thunderbiscuit
Copy link
Copy Markdown
Member

thunderbiscuit commented Mar 13, 2025

Oh funny enough I didn't mean adding the lint check in the CI but just running the other test files through haha. But I actually don't mind having it there either. However I feel like the CI run should probably fail for all other python test files now (except for yours that you ran through the linter).

Can you also add another commit that fixes the other test files (does the linter auto-correct the files if it can? Or does it simply error out with the errors and violations when you run it?)

@rustaceanrob
Copy link
Copy Markdown
Collaborator Author

rustaceanrob commented Mar 13, 2025

Yeah, it should error out. I checked out master and ran it to get the Kyoto errors, then switched to the feature branch and they passed. I think the other tests were just written well.

There is also a way to add a pre commit hook that formats the tests automatically, but couldn't get that Ruff github action to run correctly

@thunderbiscuit
Copy link
Copy Markdown
Member

thunderbiscuit commented Mar 13, 2025

Oh awesome! I'm surprised they pass because I don't know the Python standards enough and even when I run Kotlin linters on my code it tends to find a bunch little nits so... surprising that my Python code is up to snuff! That's good. Ready to go then.

Copy link
Copy Markdown
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK af17607.

@thunderbiscuit thunderbiscuit merged commit af17607 into bitcoindevkit:master Mar 13, 2025
21 checks passed
@rustaceanrob rustaceanrob deleted the python-test-kyoto-3-12 branch March 13, 2025 22:23
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

Successfully merging this pull request may close these issues.

2 participants