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

Start adding type annotations to gunicorn #2377

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MartinThoma
Copy link

See issue #2374 and issue #1393

@MartinThoma
Copy link
Author

Hi guincorn developers! First of all: A big thank you for gunicorn 🤗 I've used it for several projects and never had to think a lot about that part :-) I'm currently annotating a code base and noticed that gunicorn does not have type annotations, sooo ... here you are :-)

As I'm not familiar with the codebase, this will require some work and help of people who know gunicorn.

In case you want to read more about type annotations: I wrote an article :-)

How to get type annotations

This is how I would like to go forward and add types to gunicorn:

  1. Make sure mypy doesn't crash anymore. Decide on stub files vs in-line annotations. (In the PR you see the in-line annotations example; let me know if you think stub files would be the better idea)
  2. Integrate mypy in CI pipeline: Let me know if you prefer not to do that; note that this would only fail if mypy discovers a new problem - when new code without annotations is added, it is not checked and thus does NOT fail. Those settings can be adjusted, of course.
  3. Increase typed parts
    • Try to get rid of "type[ignore]" / Any / untyped generics
    • Type untyped functions (currently, I mainly added annotations for the config.py)

Impossible at the moment

  • Using "real" annotations everywhere (variable annotations were introduced in Pyhton 3.6)
  • Have "nice" annotations when a class returns itself (from __future__ import annotations was introduced in Python 3.7)
  • Using TypedDict instead of Dict (introduced in Python 3.8)

@MartinThoma
Copy link
Author

MartinThoma commented Jul 12, 2020

I see a lot of linting errors. I'm used to the black formatter. Would it be ok to apply it? Do you have other autoformatters?

@benoitc
Copy link
Owner

benoitc commented Jul 12, 2020

I see a lot of linting errors. I'm used to the black formatter. Would it be ok to apply it? Do you have other autoformatters?

No autoformatter is and will be used. (one readon is to keep some flexibility). If you see any pep8 issue, let's fix them in a separate commit :)

gunicorn/config.py Show resolved Hide resolved
gunicorn/config.py Outdated Show resolved Hide resolved
@benoitc
Copy link
Owner

benoitc commented Jul 13, 2020

I see a lot of linting errors. I'm used to the black formatter. Would it be ok to apply it? Do you have other autoformatters?

No autoformatter is and will be used. (one readon is to keep some flexibility). If you see any pep8 issue, let's fix them in a separate commit :)

I meant at the project level. I need to publish some style rules so anyone can adapt their auto-formatter in a way we keep the code readable and not increasing that much the number of lines like some formatter do... If you use one in your env, just make sure to apply the change on a separate commit :)

@MartinThoma
Copy link
Author

I applied black to the config.py to solve the linting issues and fixed the remaining issues. Black mostly has done two types of changes:

  • Single quotes to double quotes
  • The way of line breaks for long parametrized funtions

Current:

def foo(parameter_a,
        parameter_b,
        parameter_c):
   body

Black:

def a_function(
    parameter_a,
    parameter_b,
    parameter_c
):
    body

I think yapf would support the current style, but I'm not familiar with yapf.

@MartinThoma
Copy link
Author

@benoitc I guess you are busy with other task, but just to make it clear: I'm waiting for feedback. From my side, this PR could be merged. Once it is merged, I will try to add mypy to the CI pipeline :-)

@MartinThoma
Copy link
Author

@benoitc Did you have the time to have a look at the PR?

Copy link
Collaborator

@tilgovi tilgovi left a comment

Choose a reason for hiding this comment

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

I support the idea of adding type annotations. It would be nice to reduce the number of Any declarations. For example, the workers dict is Dict[int, Worker] and the pipe pair is a [socket, socket] tuple.

It is always preferable to separate style changes from other changes, so it would be nice to take the auto-formatting commit out of this PR. I support auto formatting, but we shouldn't apply it in this PR, and it sounds like @benoitc would need some more convincing.

@MartinThoma
Copy link
Author

@tilgovi I've added the more specific type annotations you mentioned and undid the changes black did to config. I've only applied black to that file and that is also where most of the type annotations were added.

@MartinThoma
Copy link
Author

@tiangolo Is there anything blocking this PR?

@MartinThoma
Copy link
Author

@benoitc Is there anything blocking this PR?

@tilgovi
Copy link
Collaborator

tilgovi commented Feb 16, 2021

I still like this idea and would love to add type annotations. I would prefer to use the annotation syntax, rather than comments, now that Gunicorn is Python 3 only. I'll ping people directly to see if I can get some consensus from folks about starting to add these types.

@benoitc benoitc added the Code related changes Changes related to code style that are not related to a feature label May 10, 2023
@stdedos
Copy link

stdedos commented Oct 11, 2023

Pls don't forget the py.typed marker 🙏

Also, partial stuff (like merging this MR) is better than no stuf

Also related to #2833?

@MartinThoma
Copy link
Author

I didn't get any feedback for over 3 years. I'm not going to put more effort in this.

@stdedos
Copy link

stdedos commented Oct 12, 2023

That is ... very respected ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code related changes Changes related to code style that are not related to a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants