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

Use proper typing #4

Merged
merged 6 commits into from
Dec 4, 2021
Merged

Use proper typing #4

merged 6 commits into from
Dec 4, 2021

Conversation

ItsDrike
Copy link
Contributor

@ItsDrike ItsDrike commented Dec 4, 2021

Follow PEP 484 and add type hints to all functions in the code-base. These type-hints are currently compatible with both mypy and pyright.

Now that the code-base is compatible with these, you should consider also integrating them into the automated CI/CD github workflows. (If you don't know how to, let me know if you want them added)

Note: I've renamed ormore attribute for Amount class to a more pythonic or_more name and I've also changed the parameter names for Range class from a and z to start and stop

Comment on lines 72 to 97
@staticmethod
def parse_init_args(*args, **kwargs):
s1 = inspect.Signature(
parameters=(
inspect.Parameter("repetitions", kind=inspect.Parameter.POSITIONAL_OR_KEYWORD),
inspect.Parameter("or_more", kind=inspect.Parameter.POSITIONAL_OR_KEYWORD)
)
)
s2 = inspect.Signature(
parameters=(
inspect.Parameter("minimum", kind=inspect.Parameter.POSITIONAL_OR_KEYWORD),
inspect.Parameter("maximum", kind=inspect.Parameter.POSITIONAL_OR_KEYWORD)
)
)
try:
bound = s1.bind(*args, **kwargs)
i = bound.arguments["repetitions"]
j = None
or_more = bound.arguments["or_more"]
except TypeError:
bound = s2.bind(*args, **kwargs)
i = bound.arguments["minimum"]
j = bound.arguments["maximum"]
or_more = False

return i, j, or_more
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be a bit over-the-top for what it's doing, so I will understand if instead of complex logic like this, you'd prefer this code without any overloads and just have less descriptive and less strict function parameters.

Copy link
Contributor Author

@ItsDrike ItsDrike Dec 4, 2021

Choose a reason for hiding this comment

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

Another option to avoid this mess would be to convert this into 2 separate class methods instead of having both logic for minimum and maximum or for range with optional or_more attribute. This could look like this:

class Amount:
    def __init__(i: int, j: Optional[int] = None, or_more: bool = False):
        if j is not None and or_more:
            raise ValueError("You can't specify both the maximum value and set or_more to True.")
        ...  # The original logic
        
    @classmethod
    def from_range(cls, minimum: int, maximum: int) -> "Amount":
        return cls(minimum, maximum, False)
        
    @classmethod
    def from_repetitions(cls, repetitions: int, or_more: bool = False) -> "Amount":
        return cls(repetitions, None, or_more)

However this will result in having 2 class methods acting very similarly to __init__ at which point it may not even be worth it to do this:

x = Amount(1, 15)
x = Amount.from_range(1, 15)
y = Amount(15, or_more=True)
y = Amount.from_repetitions(15, or_more=True)

The decision is up to you, I'll adapt the code to whichever option you pick.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's go with the overloading direction. It distinguishes the the arguments that can be passed explicitly rather than implying those parameters with less strict default arguments.

@GrandMoff100
Copy link
Owner

Later today I'll implement pyright into a CI/CD workflow that'll run along the same triggers as the flake8 workflow.

@GrandMoff100
Copy link
Owner

Are there any more changes that you are intending to make before merging this?

@GrandMoff100 GrandMoff100 merged commit a6e14d2 into GrandMoff100:master Dec 4, 2021
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