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

Add mypy to CI pipeline and begin typing modules #435

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gnikonorov
Copy link
Member

@gnikonorov gnikonorov commented Dec 21, 2020

Begin the mypy/typing addition as outlined in #434 by incorporating mypy and typing:

  • __init__.py
  • extras.py
  • outcome.py
  • util.py

The rest of the files will be typed in follow up PRs to not have to submit one large PR.

The mypy config was started by taking that of pytest's and then being a little stricter on untyped/incomplete function definitions and calls. You can see the full list of config options here, but I think this is a good enough starting point.

@gnikonorov gnikonorov added skip-changelog Can be missed from the changelog. Infrastructure Changes related to project infrastructure ( CI/CD, deploy mechanism, etc. ) code quality This PR has to do with improving code readability/quality ( refactoring, etc. ) labels Dec 21, 2020
@gnikonorov gnikonorov self-assigned this Dec 21, 2020
This was referenced Dec 21, 2020
@gnikonorov gnikonorov changed the title Add mypy to CI pipeline and type outcome.py Add mypy to CI pipeline and begin typing modules Dec 21, 2020
@BeyondEvil
Copy link
Contributor

Something that worries me a little bit, is that we might be raising the bar/entry for contributors by using typing. 🤔

@gnikonorov
Copy link
Member Author

Something that worries me a little bit, is that we might be raising the bar/entry for contributors by using typing. 🤔

Why do you say that? If anything I'd say it lowers the bar since people know the type of arguments immediately on reading the code and no longer have to guess at what's going on with types

Besides, it's becoming more common practice to type Python

@BeyondEvil
Copy link
Contributor

Something that worries me a little bit, is that we might be raising the bar/entry for contributors by using typing. 🤔

Why do you say that? If anything I'd say it lowers the bar since people know the type of arguments immediately on reading the code and no longer have to guess at what's going on with types

Besides, it's becoming more common practice to type Python

You pretty much answered your own question:

people know the type of arguments immediately on reading the code

Besides, it's becoming more common practice to type Python

Yes, it's becoming more common, but far from the norm yet, and to be able to read it - they have to first understand it, otherwise it's just noise that often can lead to hard to understand errors. Especially for more complex types.

@gnikonorov
Copy link
Member Author

gnikonorov commented Dec 21, 2020

It's just the typing library, and you only have complex types if you introduce them.

In my personal experience I don't think it'll complicate things. Most people are used to types (c++, Java, etc.) and the types are named almost identically to the Python objects they represent.

The only issue I can see is someone having a hard time guessing what type something is when writing new code, but I would say that the author should fully understand what they're implementing in that case before they implement it.

@@ -20,41 +28,51 @@ def extra(content, format_type, name=None, mime_type=None, extension=None):
}

Copy link
Contributor

Choose a reason for hiding this comment

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

html(), image() etc are facades to extra().

to make this more clear, i suggest to define a variable with the return type of extra() as _ExtraResult = Dict[str, Optional[str]]
and use this _ExtraResult where needed for extra(), html() and so on

@@ -1,5 +1,5 @@
try:
from . import __version
from . import __version # type: ignore

__version__ = __version.version
Copy link
Contributor

Choose a reason for hiding this comment

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

since __version is an optioanl import at the time typecheckers run,
i usggest to typehint __version__ ala

     __version__: str = __version.version 

this makes type checkers aware that __version__ is supposed to be a sting and warn/fail if it is not

@BeyondEvil
Copy link
Contributor

Could you contribute here @jeffwright13 ?

@jeffwright13
Copy link
Collaborator

jeffwright13 commented Nov 4, 2023

Could you contribute here @jeffwright13 ?

Sure

@@ -1,6 +1,7 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
# type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a directive for an IDE extension or something? Like, maybe mypy? I am familiar with typing (use it all the time), but not the details of mypy.

Copy link
Collaborator

@jeffwright13 jeffwright13 left a comment

Choose a reason for hiding this comment

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

Looks OK to me. Have not run it but the typing checks out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality This PR has to do with improving code readability/quality ( refactoring, etc. ) Infrastructure Changes related to project infrastructure ( CI/CD, deploy mechanism, etc. ) skip-changelog Can be missed from the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants