-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: attestation and committees endpoints #426
Conversation
36a6f4f
to
14487e4
Compare
from typing import Literal, Optional, Union | ||
from typing import Literal, Optional, Union, Iterator, cast, Tuple | ||
|
||
import json_stream.requests # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why import of this format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was auto-import. will change that if it's mandatory
@@ -74,11 +80,11 @@ def get_block_root(self, state_id: Union[SlotNumber, BlockRoot, LiteralState]) - | |||
@lru_cache(maxsize=1) | |||
def get_block_header(self, state_id: Union[SlotNumber, BlockRoot]) -> BlockHeaderFullResponse: | |||
"""Spec: https://ethereum.github.io/beacon-APIs/#/Beacon/getBlockHeader""" | |||
data, meta_data = self._get( | |||
data, meta_data = cast(Tuple[dict, dict], self._get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably may use native types like cast(tuple[dict, dict])
but why this cast is here at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of mypy issue python/mypy#11692 (comment)
epoch: Optional[EpochNumber] = None, | ||
index: Optional[int] = None, | ||
slot: Optional[SlotNumber] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modern syntax is like epoch: EpochNumber | None = None
) -> Tuple[dict | list, dict]: | ||
query_params: Optional[dict] = None, | ||
stream: bool = False | ||
) -> Tuple[dict | list, dict] | Response: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this return value like from different abstraction level
I'd prefer to process streaming here as it is with usual json response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you also don't need cast everywhere in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to give the ability of handling the stream to the child implementations
No description provided.