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

Type hinting #258

Closed
sobolevn opened this issue Oct 21, 2017 · 19 comments
Closed

Type hinting #258

sobolevn opened this issue Oct 21, 2017 · 19 comments
Assignees
Labels
enhancement Enhancement proposals

Comments

@sobolevn
Copy link
Collaborator

I have started to use mypy. Aaaand it is not so good right now.

Libraries lack typing support. And I know that mimesis support only python3.
Do you consider adding type hints?

There are some advantages:

  • stricter API
  • static analysis support to improve quality

There are some disadvantages:

  • type hinting was introduced in python3.5, so any version prior to that will have to use special comments or other hacks
  • a lot of manual work
  • possible API changes
@lk-geimfari
Copy link
Owner

lk-geimfari commented Oct 21, 2017

Do you mean annotations?

Something like this:

def function(num: float) -> int:
    pass

Right?

@sobolevn
Copy link
Collaborator Author

Yes.

@lk-geimfari
Copy link
Owner

I thinks that we can add annotations. What about Python 3.4?

@lk-geimfari lk-geimfari added the enhancement Enhancement proposals label Oct 21, 2017
@lk-geimfari
Copy link
Owner

lk-geimfari commented Oct 21, 2017

Seems like annotations was added in Python 3.0. And we lose nothing.

@sobolevn
Copy link
Collaborator Author

sobolevn commented Oct 21, 2017

@lk-geimfari
Copy link
Owner

@sobolevn Soooo... What shall we do? Refusing to support Python 3.4 doesn't seem to be a good idea.

@sobolevn
Copy link
Collaborator Author

It actually is in some cases. Since upgrading from 3.4 to 3.5 is painless in 90% of situations.

@lk-geimfari
Copy link
Owner

@sobolevn Let's do this!

@sobolevn
Copy link
Collaborator Author

sobolevn commented Oct 21, 2017

Please, note that variable annotations is not supported in 3.5. https://stackoverflow.com/questions/39971929/what-are-variable-annotations-in-python-3-6/39973133#39973133

So, we have to annotate methods and functions only.

@lk-geimfari
Copy link
Owner

@sobolevn I will add annotations for functions and methods. Can i hope for your help with situations which confuse me?

@lk-geimfari
Copy link
Owner

lk-geimfari commented Oct 21, 2017

Example:

    def randints(self, amount: int = None, a: int = 1, b: int = 100) -> list:
        """Generate list of random integers.

        :param amount: Amount of elements.
        :param a: Minimum value of range.
        :param b: Maximum value of range.
        :return: List of random integers.
        """

        if not amount:
            amount = 3

        return [self.randint(a, b)
                for _ in range(amount)]

Is it right that amout: int = None. Is it should be integer default value or None is okay?

@lk-geimfari
Copy link
Owner

@sobolevn How to be if the function returns multiple types using? Should we use typing module?

@sobolevn
Copy link
Collaborator Author

sobolevn commented Oct 22, 2017

In regular python without types I would say that None is perfectly fine.
But in this particular situation it is possible to change amount=None to amount: int = 3. That what it does anyway, doesn't it?

About function returning multiple types. That's a code smell for me. It is error prone for many reasons:

  • we expect tuple but list is returned, seems like not a big deal, but our old code x = list_not_tuple + (1, 2, ) will fail
  • if function could possibly return None - it is a disaster, since we will got TypeErrors with a 100% guarantee
  • and so on

So, consider not to do it.

@lk-geimfari
Copy link
Owner

@sobolevn Problem is in standard python library. json module returns both list and dict.

@lk-geimfari
Copy link
Owner

So, seems like i have found a solution.

@sobolevn
Copy link
Collaborator Author

With the provided solution there are a lot of uncovered cases, which are valid json:

  • [1, [], {}, "z"]
  • 1 and "a"

@lk-geimfari lk-geimfari mentioned this issue Oct 26, 2017
6 tasks
@emmeowzing
Copy link
Contributor

emmeowzing commented Oct 30, 2017

@lk-geimfari I've already mentioned this in #267, but if one is defaulting the argument to None, which is automatically pushed to type(None) or NoneType, then we should use typing.Optional[the_type]. In the example above, we'd have

def randints(self, amount: Optional[int] = None, a: int = 1, b: int = 100) -> list:
        """Generate list of random integers.

        :param amount: Amount of elements.
        :param a: Minimum value of range.
        :param b: Maximum value of range.
        :return: List of random integers.
        """

        if amount is None:
            amount = 3

        return [self.randint(a, b) for _ in range(amount)]

By the way, I believe I mistakenly reversed the logic of that predicate in my PR earlier :P

@lk-geimfari
Copy link
Owner

@bjd2385 I'll update code using Optional.

@lk-geimfari
Copy link
Owner

Sooo, it's done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement proposals
Projects
None yet
Development

No branches or pull requests

3 participants