Skip to content
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

fix: refactor type hint retrieval #312

Merged
merged 5 commits into from
Oct 29, 2023
Merged

Conversation

dandhlee
Copy link
Collaborator

@dandhlee dandhlee commented Aug 22, 2023

Refactors type hint retrieval when inspecting the source code. The indentation was very ugly and finally got to get around fixing it.

Given the annotation, tries to retrieve its name. For more complex types, removes ForwardRef references as they're used for type hinting, not for the user to read.

Updated goldens to verify the fix can be found in #311 - I split this PR into two for smaller PRs to review. Added goldens in this PR seem to not have been picked up from a previous PR, perhaps.

Towards b/296938464. Unblocks #311.

  • Tests pass

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Aug 22, 2023
@dandhlee dandhlee requested a review from tbpg August 22, 2023 04:08
@dandhlee dandhlee marked this pull request as ready for review August 22, 2023 04:10
@dandhlee dandhlee requested review from a team as code owners August 22, 2023 04:10
@@ -732,6 +732,52 @@ def extract_product_name(name):
return product_name


def _extract_type_name(annotation: Any) -> str:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are annotations and type hints the same? If not, I'd rename annotation to type_hint, or maybe argument_annotations. Feel free to Ack if this breaks your current naming conventions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's quite the same - annotations may hint at the type hint, however it's more like the object that includes the type hint. Probably best to stick to annotations from https://docs.python.org/3/library/inspect.html#inspect.get_annotations.

type_name[end_index+prefix_end_len:],
])

return type_name

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider adding a unit test for this new logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me add a unit test as a followup PR. Filed #318.

@@ -732,6 +732,52 @@ def extract_product_name(name):
return product_name


def _extract_type_name(annotation: Any) -> str:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dict[Any]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, tried looking into the type and it's very messy lol it returns the inspected object's class directly. I will add this to the docstring!

FYI this is what I got from a small sample:

type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._GenericAlias'>
type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._GenericAlias'>
type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._GenericAlias'>
type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._GenericAlias'>
type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._UnionGenericAlias'>
type: <class 'typing._CallableType'>
type: <class 'str'>

Copy link
Collaborator Author

@dandhlee dandhlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review!

@@ -732,6 +732,52 @@ def extract_product_name(name):
return product_name


def _extract_type_name(annotation: Any) -> str:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's quite the same - annotations may hint at the type hint, however it's more like the object that includes the type hint. Probably best to stick to annotations from https://docs.python.org/3/library/inspect.html#inspect.get_annotations.

@@ -732,6 +732,52 @@ def extract_product_name(name):
return product_name


def _extract_type_name(annotation: Any) -> str:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, tried looking into the type and it's very messy lol it returns the inspected object's class directly. I will add this to the docstring!

FYI this is what I got from a small sample:

type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._GenericAlias'>
type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._GenericAlias'>
type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._GenericAlias'>
type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._GenericAlias'>
type: <class 'typing._CallableGenericAlias'>
type: <class 'typing._UnionGenericAlias'>
type: <class 'typing._CallableType'>
type: <class 'str'>

type_name[end_index+prefix_end_len:],
])

return type_name
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me add a unit test as a followup PR. Filed #318.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants