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 author parsing for BSZ #518

Merged
merged 3 commits into from
May 17, 2024
Merged

Fix author parsing for BSZ #518

merged 3 commits into from
May 17, 2024

Conversation

MaxDall
Copy link
Collaborator

@MaxDall MaxDall commented May 16, 2024

This PR replaces the current generic implementation of author parsing for BSZ as well as replacing the current test case with a more challenging one.

@MaxDall MaxDall requested a review from addie9800 May 16, 2024 11:42
addie9800
addie9800 previously approved these changes May 16, 2024
Copy link
Collaborator

@addie9800 addie9800 left a comment

Choose a reason for hiding this comment

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

Thanks for the update 👍

@@ -85,7 +88,7 @@ def get_value_by_key_path(self, key_path: List[str], default: Any = None) -> Opt
tmp = nxt
return tmp

def bf_search(self, key: str, depth: Optional[int] = None, default: Any = None) -> Optional[Any]:
def bf_search(self, key: str, depth: Optional[int] = None, default: Optional[_T] = None) -> Union[Any, _T]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does it make sense to update the return value to Union[Any, _T]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So mypy does know that if a default is given, at least that type is returned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok. I didn't know mypy was able to make that connection. Thanks

@addie9800 addie9800 dismissed their stale review May 16, 2024 12:27

I just wanted to comment

@MaxDall MaxDall merged commit b7df009 into master May 17, 2024
5 checks passed
@MaxDall MaxDall deleted the fix-bsz-author-parsing branch May 17, 2024 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants