Skip to content

Annotate celery/security#7401

Closed
Kludex wants to merge 6 commits intocelery:mainfrom
Kludex:annotate/security
Closed

Annotate celery/security#7401
Kludex wants to merge 6 commits intocelery:mainfrom
Kludex:annotate/security

Conversation

@Kludex
Copy link
Copy Markdown
Contributor

@Kludex Kludex commented Apr 3, 2022

I had to remove utils.py and serialization.py from the mypy files, as there are some mypy errors that are sensitive i.e. I'd need to implement further changes (which can be considered breaking changes). I don't want to implement breaking changes on annotation PRs, so I'll open separate ones later on. If you want to know which are the sensitive ones I'm mentioning, you just need to add those two files on the pyproject.toml, and you'll understand the changes that are needed.

@Kludex Kludex changed the title Annotate/security Annotate celery/security Apr 3, 2022
@Kludex Kludex marked this pull request as ready for review April 3, 2022 07:16
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 3, 2022

This pull request introduces 1 alert and fixes 2 when merging cd70b68 into 1ccd887 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

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

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2022

Codecov Report

Attention: Patch coverage is 41.46341% with 24 lines in your changes missing coverage. Please review.

Project coverage is 89.18%. Comparing base (1ccd887) to head (22a231e).
Report is 847 commits behind head on main.

Files with missing lines Patch % Lines
celery/security/serialization.py 36.84% 11 Missing and 1 partial ⚠️
celery/security/__init__.py 36.36% 6 Missing and 1 partial ⚠️
celery/security/key.py 50.00% 2 Missing and 1 partial ⚠️
celery/security/utils.py 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7401      +/-   ##
==========================================
- Coverage   89.30%   89.18%   -0.12%     
==========================================
  Files         138      138              
  Lines       16762    16790      +28     
  Branches     2451     2458       +7     
==========================================
+ Hits        14969    14974       +5     
- Misses       1561     1580      +19     
- Partials      232      236       +4     
Flag Coverage Δ
unittests 89.17% <41.46%> (-0.12%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

else:
from typing import Literal

_Serializer = Literal["json", "msgpack", "yaml", "pickle"]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is kind of tricky, because the user can also add custom serializers. Meaning that the annotation should be str instead. I'm open to change it.

Also, not related, but I have two _Serializer in this PR, on different files. I accept suggestion on where to put them, if duplicated is not wanted.

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 3, 2022

This pull request fixes 2 alerts when merging 22a231e into 1ccd887 - view on LGTM.com

fixed alerts:

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

@Kludex Kludex closed this by deleting the head repository Jul 29, 2024
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