-
Notifications
You must be signed in to change notification settings - Fork 64
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
Added support for fetching data with protocol buffers #1728
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1728 +/- ##
==========================================
- Coverage 78.21% 73.68% -4.53%
==========================================
Files 242 244 +2
Lines 12787 12769 -18
==========================================
- Hits 10001 9409 -592
- Misses 2786 3360 +574
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -16,13 +16,15 @@ pytest-xdist | |||
vega_datasets | |||
backoff | |||
altair | |||
icecream |
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? 😄
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.
Don't get me wrong, I love this tool - I just don't see it used anywhere
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 annoying to install it over and over locally, same as grpc_tools - Not used by the code/tests itself but helps with development.
@@ -97,7 +98,8 @@ def get_single_page( | |||
types: Optional[Iterable[str]], | |||
query: Optional["NQLQuery"], | |||
searching_after: Optional[str], | |||
) -> Any: | |||
use_proto: Optional[bool] = 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.
Why is this optional? Cannot be just use_proto: bool = False
?
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.
None
means -> keep what environment variable says. True
/False
will override it. Used for testing purposes.
@@ -1062,7 +1069,9 @@ def search_leaderboard_entries( | |||
ascending: bool = False, | |||
progress_bar: Optional[ProgressBarType] = None, | |||
step_size: Optional[int] = None, | |||
use_proto: Optional[bool] = 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.
Same here, maybe just bool
?
self, | ||
container_id: str, | ||
container_type: ContainerType, | ||
use_proto: Optional[bool] = 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.
And here (just bool
)
Before submitting checklist