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

remove implementation from BlockRecordProtocol.is_transaction_block() #18948

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

altendky
Copy link
Contributor

Purpose:

Current Behavior:

New Behavior:

Testing Notes:

@altendky altendky 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 Nov 27, 2024
Copy link
Contributor

@AmineKhaldi AmineKhaldi left a comment

Choose a reason for hiding this comment

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

Thank you.

@altendky altendky marked this pull request as ready for review November 27, 2024 20:16
@altendky altendky requested a review from a team as a code owner November 27, 2024 20:16
@altendky altendky requested a review from emlowe November 27, 2024 20:16
@arvidn
Copy link
Contributor

arvidn commented Nov 28, 2024

would mypy tell us if a type doesn't implement this method when it's passed as implementing this protocol?
There's no description of the PR,so I assume all types that implement this protocol already have their own implementations, and that they are compatible. is that right?

@arvidn
Copy link
Contributor

arvidn commented Nov 28, 2024

is it better to have this implemented in each class rather than in the protocol?

@altendky
Copy link
Contributor Author

I generally avoid inheriting from protocols and thus implementing on the protocol itself doesn't do anything. I don't think mypy would notice anything around this change. Since this is a protocol, mypy doesn't care that there's no return in the method. As far as other classes checked against this, mypy wouldn't care about the implementation, just the signature.

So no, I don't think mypy would notice anything changing about this.

A search only shows this protocol being used for hints, not inheritance.

The pr was just intended to clean up this seemingly unused implementation code on the interface-defining protocol.

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.

3 participants