-
Notifications
You must be signed in to change notification settings - Fork 42
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
[IO-1104][internal] DatasetQuery #616
Conversation
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 don't see anything that is blocking. Some stuff I think we need to discuss and work-out for future though.
@@ -13,7 +13,7 @@ def get_team(client: Client, team_slug: Optional[str] = None) -> Team: | |||
|
|||
|
|||
def get_team_members(client: Client) -> Tuple[List[TeamMember], List[Exception]]: | |||
response = client.get(f"/memberships") | |||
response = client.get("/memberships") |
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.
press f to pay respects
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 gen-z-ers and your crazy references 😛
@@ -19,6 +19,9 @@ class Release(DefaultDarwin): | |||
|
|||
name: str | |||
|
|||
def __str__(self) -> str: |
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.
Is there a purpose to overwriting this? Pydantic objects already print to console?
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 is a fair point, it was a workaround for a listcomp, but unnecessary really. The thing I was addressing was that the structure
{
"name": "release name"
}
which we get from the API, is kinda over complicated.
So, I was making it so that it could just be release_name
but also, this isn't actually needed right now, I amended the bit of code that was relying on it.
def collect(self, client: Client) -> List[Dataset]: | ||
datasets, exceptions = list_datasets(client) | ||
if exceptions: | ||
# TODO: print and or raise exceptions, tbd how we want to handle this |
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.
We should definitely have a discussion about exceptions soon, figure out where we want to handle them, collecting exceptions into lists, logging (somewhat seperate but related)
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.
Yeah, so the issue we have is whether we are withing a CLI or SDK context, I suppose. We could maybe add console
member to client
so that CLI usage would be detectable, or we could use sys
to detect the call stack, but that may be less x-plat compatible.
Problem
Need to implement meta queries for Datasets
Solution
Have implemented meta queries in same style to Teams for Datasets.
Changelog
Introduced DatasetQuery and tests
Refactored
list_datasets
to use monad error styles.