Skip to content

Conversation

richardkiss
Copy link
Contributor

Fix some issues that ruff identifies starting only recently (probably due to ruff improving).

@richardkiss richardkiss requested a review from a team as a code owner July 8, 2025 00:00
@richardkiss richardkiss added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes labels Jul 8, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses new linter warnings from ruff by tightening split operations, improving parsing logic, and updating test syntax.

  • Added maxsplit=1 to various split calls to prevent over-splitting.
  • Refactored a directory name check in block_tools.py for clarity.
  • Enhanced CLI argument parsing for ip:port inputs and standardized test error assertion syntax.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
chia/util/virtual_project_analysis.py Added maxsplit=1 to identifier.split( to satisfy linter warnings.
chia/simulator/block_tools.py Broke out split(..., maxsplit=1) calls and adjusted line breaks.
chia/cmds/peer_funcs.py Switched to split(":", maxsplit=1) and validated the ip:port pair.
chia/cmds/configure.py Replaced .index with split(":", maxsplit=1) to parse host and port.
chia/_tests/plot_sync/test_sender.py Converted pytest.raises to context-manager form.
Comments suppressed due to low confidence (4)

chia/simulator/block_tools.py:1738

  • [nitpick] Consider extracting the common base directory logic for plot_dir_name into a variable to avoid repeated split calls and improve readability.
            or root_dir.parts[-1] == plot_dir_name.split("\\", maxsplit=1)[0]

chia/cmds/peer_funcs.py:11

  • [nitpick] Consider using a robust parsing function or the ipaddress module to properly handle IPv6 addresses and host:port splitting instead of manual split logic.
    ip_port = add_connection.split(":", maxsplit=1)

chia/cmds/configure.py:45

  • If set_node_introducer does not contain a colon, this code silently skips setting host and port without feedback; add an else branch or input validation to inform the user of invalid format.
                host_port = set_node_introducer.split(":", maxsplit=1)

chia/cmds/configure.py:57

  • Similarly, for set_farmer_peer, if no colon is present, the code will silently do nothing; add an else clause to handle invalid input and notify the user.
                host_port = set_farmer_peer.split(":", maxsplit=1)

arvidn
arvidn previously approved these changes Jul 8, 2025
@arvidn
Copy link
Contributor

arvidn commented Jul 8, 2025

coverage report:

chia/_tests/plot_sync/test_sender.py (100%)
chia/cmds/configure.py (0.0%): Missing lines 45-47,57-59
chia/cmds/peer_funcs.py (0.0%): Missing lines 11-12,15
chia/util/virtual_project_analysis.py (100%)

@richardkiss
Copy link
Contributor Author

coverage report:

chia/_tests/plot_sync/test_sender.py (100%)
chia/cmds/configure.py (0.0%): Missing lines 45-47,57-59
chia/cmds/peer_funcs.py (0.0%): Missing lines 11-12,15
chia/util/virtual_project_analysis.py (100%)

Hmm, yeah, that ain't good. I suppose I need to actually at least try this.

@cmmarslender
Copy link
Member

Added coverage diff label for now, since convo seems like we might try and resolve those (and it was unable to be added automatically since it was from a fork).
If coverage issues end up ok to ignore, just remove the label as usual

Copy link

coveralls-official bot commented Jul 9, 2025

Pull Request Test Coverage Report for Build 16209809517

Details

  • 23 of 40 (57.5%) changed or added relevant lines in 6 files are covered.
  • 29 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.007%) to 91.354%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/cmds/peer_funcs.py 1 8 12.5%
chia/cmds/configure.py 1 11 9.09%
Files with Coverage Reduction New Missed Lines %
chia/rpc/rpc_server.py 1 88.54%
chia/_tests/core/util/test_file_keyring_synchronization.py 1 96.88%
chia/wallet/wallet_node.py 1 87.67%
chia/farmer/farmer_api.py 2 94.36%
chia/timelord/timelord.py 2 80.79%
chia/_tests/core/util/test_lockfile.py 22 77.31%
Totals Coverage Status
Change from base Build 16206121861: 0.007%
Covered Lines: 102385
Relevant Lines: 111954

💛 - Coveralls

@emlowe
Copy link
Contributor

emlowe commented Jul 9, 2025

There is a dependabot PR to update ruff: #19793

I think it would be useful to see if that introduces any further changes.

@richardkiss
Copy link
Contributor Author

richardkiss commented Jul 9, 2025

There is a dependabot PR to update ruff: #19793

I think it would be useful to see if that introduces any further changes.

I tried that. There are some errors in some very dumb test types PYI016. One is Optional[Optional[uint8]] in test_streamable which conceivably could actually cause the thing to be streamed differently than Optional[uint8] (since it could embed TWO boolean flags switching between None and the internal alternate). What a mess if so because Optional[uint8] is the same type as Optional[Optional[uint8]] everywhere else!!

This patch lets us go to 0.12.1. Let's try to get this one through first.

arvidn
arvidn previously approved these changes Jul 10, 2025
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks reasonable. feel free to resolve my tickets if you don't think they need to be addressed

@richardkiss
Copy link
Contributor Author

I think it's ready to be merged.

@cmmarslender cmmarslender merged commit 7cee2b2 into Chia-Network:main Jul 14, 2025
514 of 516 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants