Annotate celery/exceptions.py#7396
Annotate celery/exceptions.py#7396Kludex wants to merge 9 commits intocelery:mainfrom Kludex:annotate/celery/exceptions.py
celery/exceptions.py#7396Conversation
Kludex
commented
Apr 2, 2022
- Related to Plan to annotate Celery #7394
for more information, see https://pre-commit.ci
|
This pull request introduces 2 alerts when merging 51c289e into fbda008 - view on LGTM.com new alerts:
|
…ery into annotate/celery/exceptions.py
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7396 +/- ##
==========================================
- Coverage 89.30% 86.98% -2.32%
==========================================
Files 138 148 +10
Lines 16762 18464 +1702
Branches 2451 2943 +492
==========================================
+ Hits 14969 16061 +1092
- Misses 1561 2124 +563
- Partials 232 279 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
auvipy
left a comment
There was a problem hiding this comment.
please rebase & would be great if some unit test coverage can be increased
|
I don't want to add tests on those PRs, as I'm not including any functionally I don't think it makes sense (or is fair). That being said... I'm willing to create a plan to increase the coverage of the whole project to 100%, analogous to the plan created to add annotations (progressive, and by file(s)). I did Kludex/uvicorn#102 for uvicorn. If you agree, I can create a similar issue, and setup for it as well (so the coverage doesn't drop, only goes up). |
|
This pull request fixes 1 alert when merging a516751 into 314d014 - view on LGTM.com fixed alerts:
|
| #: Time of retry (ETA), either :class:`numbers.Real` or | ||
| #: :class:`~datetime.datetime`. |
There was a problem hiding this comment.
I've removed part of the comment, as it's annotated now, but I'm not sure if it has side effects on the docs?
| #: :class:`~datetime.datetime`. | ||
| when = None | ||
| #: Time of retry (ETA) | ||
| when: Optional[Union[datetime, numbers.Number]] = None |
There was a problem hiding this comment.
I've used numbers.Number instead of numbers.Real because there's a conditional on humanize() to check if it's an instance of numbers.Number. I can change it tho.
There was a problem hiding this comment.
I think you can leave it like that, but keep the original doc with the :class:numbers.Real doc, being a more specialized doc compared to the "general" annotation as numbers.Number.
What do you think?
| """An issue writing to the backend.""" | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| def __init__(self, *args: str, **kwargs: str) -> None: |
There was a problem hiding this comment.
There are several options here:
- Use
Unpack(provided on PEP 646 viatyping_extensions). - Just replace
**kwargstostate: str, task_id: str- This is considered a breaking change, and I don't want to do anything besides annotating.
|
This pull request introduces 1 alert when merging 4817496 into 1ccd887 - view on LGTM.com new alerts:
|
Nusnus
left a comment
There was a problem hiding this comment.
Left one comment but not something critical to change, food for thought.
Besides, LGTM too.
| #: :class:`~datetime.datetime`. | ||
| when = None | ||
| #: Time of retry (ETA) | ||
| when: Optional[Union[datetime, numbers.Number]] = None |
There was a problem hiding this comment.
I think you can leave it like that, but keep the original doc with the :class:numbers.Real doc, being a more specialized doc compared to the "general" annotation as numbers.Number.
What do you think?
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>