Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

WIP: major clean ups for the codebase #86

Merged
merged 2 commits into from
Sep 14, 2023
Merged

WIP: major clean ups for the codebase #86

merged 2 commits into from
Sep 14, 2023

Conversation

hozan23
Copy link
Contributor

@hozan23 hozan23 commented Jun 28, 2023

No description provided.

@lemonyte
Copy link
Contributor

I see the majority of the changes are adding Union[<type>, None] type hints for parameters. The recommended way to type hint this is to use typing.Optional, which is basically an alias for Union[<type>, None]. Also, a lot of the changes are duplicated in #69, perhaps it would be best to review that PR and see what could be merged there?

@hozan23
Copy link
Contributor Author

hozan23 commented Jul 11, 2023

I see the majority of the changes are adding Union[, None] type hints for parameters. The recommended way to type hint this is to use typing.Optional, which is basically an alias for Union[, None]. Also, a lot of the changes are duplicated in #69, perhaps it would be best to review that PR and see what could be merged there?

I am not sure about the ideal way of using typing hints in Python. I think Union and Optional are equivalent. In the future, we will use type | None instead of both. Also, in my opinion, Union is more like the new typing hint.

@pomdtr
Copy link

pomdtr commented Jul 11, 2023

I am not sure about the ideal way of using typing hints in Python. I think Union and Optional are equivalent. In the future, we will use type | None instead of both. Also, in my opinion, Union is more like the new typing hint.

Optional is the way to go. They are virtually equivalent (see https://peps.python.org/pep-0645/), but optional is a more readable way to express the same thing.

@hozan23 hozan23 requested a review from aavshr September 13, 2023 17:01
@cclauss
Copy link
Contributor

cclauss commented Sep 13, 2023

It is probably better to fix the failing tests BEFORE merging more pull requests.

@hozan23
Copy link
Contributor Author

hozan23 commented Sep 13, 2023

Hey @cclauss thanks for your advice.

@hozan23 hozan23 merged commit 06d36da into deta:master Sep 14, 2023
0 of 5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants