-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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): autogenerate urn types #9257
Conversation
URN_TYPES: Dict[str, Type["_SpecificUrn"]] = {} | ||
|
||
|
||
def _split_entity_id(entity_id: str) -> List[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.
Some doctests / examples of what this is doing would be nice. I eventually got it but took a sec. Also kinda odd to me that it basically ignores parentheses, besides checking that there's the right amount of them in an appropriate order.
It's less efficient but think it'd be clearer to break this up:
PARENS = ["(", ")"]
def _split_entity_id(entity_id: str) -> List[str]:
if not (entity_id.startswith("(") and entity_id.endswith(")")):
return [entity_id]
parens = [v for v in entity_id if v == "(" or v == ")"]
if not _parens_valid(parens):
raise InvalidUrnError(...)
return entity_id[1:-1].split(",")
I think that's what this function is doing?
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 was copied from the existing urn code, so I'm inclined to leave it as is since it seems to work
|
||
# TODO: Add handling for url encoded urns e.g. urn%3A ... | ||
|
||
if not urn_str.startswith("urn:li:"): |
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 use the constants above here? Or that's just overkill
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 it's fine
Also fixes an issue where the models documentation wasn't showing up properly with sphinx.
Remaining items:
Checklist