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

explicitly collect api methods instead of later using getattr #18800

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Oct 30, 2024

Purpose:

i never really felt good about the getattr() down in ws_connection.py. beyond just addressing that, this gives each api implementation (FullNodeApi etc) a specific object holding the api definition. a small step towards having a well formed definition of our api rather than having to iterate misc maybe-not-part-of-the-api methods anywhere you need to discover this information.

Current Behavior:

New Behavior:

Testing Notes:

@altendky altendky added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup labels Oct 30, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Oct 30, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Oct 30, 2024
@altendky altendky marked this pull request as ready for review November 1, 2024 16:24
@altendky altendky requested a review from a team as a code owner November 1, 2024 16:24
Quexington
Quexington previously approved these changes Nov 1, 2024
Copy link
Contributor

@Quexington Quexington left a comment

Choose a reason for hiding this comment

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

I'm not the most familiar with this section of the code, but I understand and support the general idea. I made a couple of the monkeypatched areas of the tests I think and assure you I put less thought into it than was likely put into fixing it here.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Nov 1, 2024
Copy link
Contributor

github-actions bot commented Nov 1, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Nov 6, 2024
Copy link
Contributor

github-actions bot commented Nov 6, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link

coveralls-official bot commented Nov 6, 2024

Pull Request Test Coverage Report for Build 11713006287

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 298 of 298 (100.0%) changed or added relevant lines in 18 files are covered.
  • 267 unchanged lines in 24 files lost coverage.
  • Overall coverage decreased (-0.008%) to 90.931%

Files with Coverage Reduction New Missed Lines %
chia/_tests/simulation/test_simulation.py 1 96.45%
chia/util/config.py 1 84.62%
chia/server/node_discovery.py 1 80.55%
chia/_tests/wallet/test_wallet_node.py 1 99.79%
chia/consensus/block_creation.py 1 99.58%
chia/rpc/rpc_server.py 1 88.67%
chia/timelord/timelord_launcher.py 1 90.0%
chia/_tests/clvm/test_puzzles.py 1 99.24%
chia/_tests/blockchain/test_blockchain.py 1 99.26%
chia/server/ws_connection.py 2 88.99%
Totals Coverage Status
Change from base Build 11706933258: -0.008%
Covered Lines: 102925
Relevant Lines: 112959

💛 - Coveralls


@metadata.request(request_type=ProtocolMessageTypes.handshake)
async def f(self: object, request: RequestTransaction) -> None:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pass
pass # pragma: no cover

pass

async def g(self: object, request: RequestTransaction) -> None:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pass
pass # pragma: no cover

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 Cleanup Code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants