-
Notifications
You must be signed in to change notification settings - Fork 3k
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(sdk): add scaffolding for sdk v2 #12554
Conversation
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
|
||
|
||
class Container( | ||
HasPlatformInstance, |
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.
👍 love this mix-in design change. this also makes object initialization simpler, by hiding specific schema class name in setter!
from datahub.utilities.str_enum import StrEnum | ||
|
||
|
||
class KnownAttribution(StrEnum): |
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.
should we consider custom attributions beyond the listed ones here?
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.
yes we will - this is not the final form. attribution will likely be expanded upon a bunch, but this works well enough for an initial experimental impl
# TODO: call this custom properties? | ||
extra_properties: Optional[Dict[str, str]] = 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.
I think our users are familiar with custom properties, so no need to change this.
mock_graph.get_urns_by_filter.return_value = ["urn:li:corpuser:user"] | ||
assert client.resolve.user(name="User") == CorpUserUrn("urn:li:corpuser:user") |
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 this the expected usage?
to fetch the URN so later we can fetch other data for that URN?
if so, this is promoting a two-round-trip for any data fetching for something that could be done in a single round 🤔
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 purely for resolving which can be useful for e.g. populating the ownership field.
dataset.add_owner(client.resolve.user(email=...))
I don't expect people to be using this method in isolation - that's only for testing.
Checklist