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 the pipeline #7383

Merged
merged 8 commits into from Apr 2, 2022
Merged

Conversation

Kludex
Copy link
Contributor

@Kludex Kludex commented Mar 29, 2022

Thanks @atombrella for the initial work!

This PR adds mypy to tox and to the pipeline. A couple of differences between this PR and the previous one:

  • Apply mypy per file. Meaning that we can do progressive work just adding a new file to the pyproject.toml.
  • Use pyproject.toml instead of mypy.ini.
  • disallow_untyped_defs is True now.

This PR is intended to be minimal, so we can start working per files basis. I've reused the previous branch, so there are also some small type annotation fixes, which are not required for this PR.

atombrella and others added 3 commits February 28, 2022 13:34
This is a simple bootstrap of the process, adding some types to a
few selected functions, based on comment annotations. MyPy is chosen as the
default static analyzer for the types.
@Kludex Kludex changed the title Typing/annotations v2 Add mypy to the pipeline Mar 29, 2022
@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #7383 (7617ec3) into master (8423c67) will decrease coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #7383      +/-   ##
==========================================
- Coverage   89.33%   89.30%   -0.03%     
==========================================
  Files         138      138              
  Lines       16781    16762      -19     
  Branches     2451     2451              
==========================================
- Hits        14991    14970      -21     
- Misses       1559     1560       +1     
- Partials      231      232       +1     
Flag Coverage Δ
unittests 89.30% <85.71%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
celery/__main__.py 0.00% <0.00%> (ø)
celery/events/state.py 97.22% <100.00%> (-0.03%) ⬇️
celery/utils/collections.py 85.84% <100.00%> (+0.15%) ⬆️
celery/backends/asynchronous.py 67.53% <0.00%> (-2.17%) ⬇️
celery/bin/worker.py 45.31% <0.00%> (-1.47%) ⬇️
celery/result.py 96.28% <0.00%> (-0.07%) ⬇️
celery/beat.py 91.54% <0.00%> (-0.05%) ⬇️
celery/app/defaults.py 97.33% <0.00%> (ø)
celery/contrib/abortable.py 100.00% <0.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 095cd78...7617ec3. Read the comment docs.

@Kludex Kludex mentioned this pull request Mar 29, 2022
@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2022

This pull request fixes 3 alerts when merging bdee5e0 into 24f22a5 - view on LGTM.com

fixed alerts:

  • 1 for Non-exception in 'except' clause
  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@povilasb
Copy link

Nice, type hints are much appreciated when navigating the code base.
Thank you!

@Kludex
Copy link
Contributor Author

Kludex commented Mar 29, 2022

Yep. As soon as this gets in, I'll organize a similar issue as I did on uvicorn: encode/uvicorn#998

Then we can start working on it, in an incremental way :)

@auvipy auvipy self-requested a review April 2, 2022 05:48
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

good job

celery/contrib/testing/worker.py Outdated Show resolved Hide resolved
@Kludex
Copy link
Contributor Author

Kludex commented Apr 2, 2022

All the annotations besides __main__.py are not needed for this PR. If you want, I can remove the other ones.

@lgtm-com
Copy link

lgtm-com bot commented Apr 2, 2022

This pull request introduces 2 alerts and fixes 1 when merging e2eda10 into 24f22a5 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

fixed alerts:

  • 1 for Unused import

celery/utils/saferepr.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Apr 2, 2022

This pull request fixes 1 alert when merging 7617ec3 into 24f22a5 - view on LGTM.com

fixed alerts:

  • 1 for Non-exception in 'except' clause

@Kludex
Copy link
Contributor Author

Kludex commented Apr 2, 2022

I've just found out about this: encode/uvicorn#1437 (comment)

Might be useful for celery as well.

@auvipy auvipy merged commit fbda008 into celery:master Apr 2, 2022
@auvipy auvipy added the CI label Apr 2, 2022
@Kludex
Copy link
Contributor Author

Kludex commented Apr 2, 2022

Thanks @auvipy ! :)

I'm going to create an issue so we can organize the work towards a fully annotated celery! 🙌

@auvipy
Copy link
Member

auvipy commented Apr 2, 2022

we can just modify this #7258

@Kludex Kludex deleted the typing/annotations-v2 branch April 2, 2022 13:40
@Kludex
Copy link
Contributor Author

Kludex commented Apr 2, 2022

@auvipy I didn't modify it because I'd like to have rights to edit. See the created issue: #7394

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

Successfully merging this pull request may close these issues.

None yet

4 participants