Type annotations for `celery/apps/beat.py` by max-muoto · Pull Request #8108 · celery/celery

max-muoto · 2023-03-05T18:46:39Z

Note: Before submitting this pull request, please review our contributing
guidelines.

Description

Type annotations for celery/apps/beat.py as a part of #7394.

* Annotate text * fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix * fix * Add to pyproject * Fix * remove comment * Small fix * remove comment * remoev unused arg * Fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix * fix * build fix * type fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * pytest==7.2.2 (celery#8106) * Fix * Fix * Type checking fix * Update celery/utils/text.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Asif Saif Uddin <auvipy@gmail.com>

codecov · 2023-03-05T18:51:41Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (32a83e2) 87.02% compared to head (5db7eea) 87.01%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8108      +/-   ##
==========================================
- Coverage   87.02%   87.01%   -0.01%     
==========================================
  Files         148      148              
  Lines       18445    18449       +4     
  Branches     2515     2515              
==========================================
+ Hits        16051    16054       +3     
- Misses       2116     2118       +2     
+ Partials      278      277       -1

Flag	Coverage Δ
unittests	`86.99% <100.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
celery/apps/beat.py	`91.02% <100.00%> (+0.48%)`	⬆️
celery/schedules.py	`97.53% <100.00%> (ø)`
celery/backends/asynchronous.py	`64.93% <0.00%> (-0.44%)`	⬇️
celery/app/base.py	`96.72% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

celery/apps/beat.py

max-muoto · 2023-03-05T19:05:00Z

celery/apps/beat.py

        )

-    def install_sync_handler(self, service):
+    def install_sync_handler(self, service: Service) -> None:


Pre-commit was replacing quoted type hints. Is this intended? I think it would be useful to allow them, so that I could conditionally import types with if TYPE_CHECKING.

Nusnus · 2023-03-05T19:12:50Z

The CI was stuck - on it

for more information, see https://pre-commit.ci

max-muoto · 2023-03-05T19:30:07Z

The CI was stuck - on it

Hmm, not able to replicate locally. Do you have any debugging advice? Never really seen a CI just get stuck.

Nusnus · 2023-03-05T20:07:11Z

The CI was stuck - on it

Hmm, not able to replicate locally. Do you have any debugging advice? Never really seen a CI just get stuck.

Tbh "stuck" isn't accurate, it was pending/waiting due to a time limit if I'm not mistaken.

I've stopped the run and will re-run soon. It should allow everything to run smoothly.

max-muoto · 2023-03-05T20:08:34Z

I see, thanks!

…

On Sun, Mar 5, 2023 at 2:07 PM Tomer Nosrati ***@***.***> wrote: The CI was stuck - on it Hmm, not able to replicate locally. Do you have any debugging advice? Never really seen a CI just get stuck. Tbh "stuck" isn't accurate, it was pending/waiting due to a time limit if I'm not mistaken. I've stopped the run and will re-run soon. It should allow everything to run smoothly. — Reply to this email directly, view it on GitHub <#8108 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJZZWA5R7IGCTVUYD7XLV3DW2TW7TANCNFSM6AAAAAAVQKIHBQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Nusnus · 2023-03-05T20:11:41Z

Thank You friend!
I personally like annotations so it's a great PR IMHO.

And don't worry, I got my eyes on the CI. If something happens I'll comment/fix etc.

celery/apps/beat.py

Nusnus · 2023-03-05T21:37:04Z

celery/apps/beat.py

+    def app(self) -> Celery:
+        if self._app:
+            return self._app
+        raise ValueError("Beat must provided a Celery app.")


Added lines #L83 - L84 were not covered by tests

Can you add a unit test for this please?

Will do for both!

Fixed a type hint (scheduler/scheduler_cls), and added a test that covers both.

I seem to have some trouble running unit tests locally, pytest must be caching (or something else, not sure) as the new field on the Beat class isn't even picked up when inspecting with pdb. Do you have any ideas here?

Nusnus · 2023-03-05T21:37:21Z

celery/apps/beat.py

+
+    @app.setter
+    def app(self, app: Celery) -> None:
+        self._app = app


Added line #L88 was not covered by tests

Can you add a unit test for this please?

for more information, see https://pre-commit.ci

Nusnus · 2023-03-05T22:59:37Z

https://github.com/celery/celery/actions/runs/4338258344/jobs/7574969608#step:8:32984

max-muoto · 2023-03-08T02:57:37Z

@Nusnus Modified it a way that doesn't require a setter/getter so removed the test, if you want to take another look.

It turns out there was no need to explicitly set the app field in the class to None, since it gets overridden anyway when grabbing it from the app. If it isn't set, and no app was provided for the init, then it doesn't make a difference since it would have run into an attribute error trying to call a method on app anyway.

celery/apps/beat.py

for more information, see https://pre-commit.ci

Nusnus

@auvipy LGTM

max-muoto and others added 5 commits March 5, 2023 12:45

Type annotations

3a61135

small change

9e75451

unused import

dd042f6

Merge branch 'celery:main' into beat.py-annotations

e00b182

Pre-commit fix

442d3d8

max-muoto commented Mar 5, 2023

View reviewed changes

celery/apps/beat.py Outdated Show resolved Hide resolved

max-muoto commented Mar 5, 2023

View reviewed changes

value error change

8fc9590

max-muoto marked this pull request as ready for review March 5, 2023 19:05

max-muoto and others added 2 commits March 5, 2023 13:15

Fix

f0bc72f

[pre-commit.ci] auto fixes from pre-commit.com hooks

d80481f

for more information, see https://pre-commit.ci

Nusnus approved these changes Mar 5, 2023

View reviewed changes

Nusnus added the Category: Documentation label Mar 5, 2023

Nusnus added this to the 5.3 milestone Mar 5, 2023

max-muoto and others added 3 commits March 5, 2023 16:13

Unit test

b23531e

[pre-commit.ci] auto fixes from pre-commit.com hooks

fe59cd2

for more information, see https://pre-commit.ci

Comment fix

1df832f

Nusnus self-requested a review March 5, 2023 23:00

max-muoto and others added 2 commits March 7, 2023 19:53

Merge branch 'celery:main' into beat.py-annotations

2d7387b

Fix

f5c6709

auvipy requested changes Mar 8, 2023

View reviewed changes

celery/apps/beat.py Outdated Show resolved Hide resolved

max-muoto and others added 5 commits March 8, 2023 00:33

Future import

3d3bd83

[pre-commit.ci] auto fixes from pre-commit.com hooks

001c92e

for more information, see https://pre-commit.ci

revert

1afdd2f

format fix

2e779a3

[pre-commit.ci] auto fixes from pre-commit.com hooks

30a074d

for more information, see https://pre-commit.ci

Nusnus approved these changes Mar 8, 2023

View reviewed changes

Update celery/apps/beat.py

5db7eea

auvipy approved these changes Mar 9, 2023

View reviewed changes

auvipy merged commit 93bccdc into celery:main Mar 9, 2023

Uh oh!

Conversation

max-muoto commented Mar 5, 2023

Description

Uh oh!

codecov bot commented Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

max-muoto Mar 5, 2023

Choose a reason for hiding this comment

Uh oh!

Nusnus commented Mar 5, 2023

Uh oh!

max-muoto commented Mar 5, 2023

Uh oh!

Nusnus commented Mar 5, 2023

Uh oh!

max-muoto commented Mar 5, 2023 via email

Uh oh!

Nusnus commented Mar 5, 2023

Uh oh!

Uh oh!

Nusnus Mar 5, 2023

Choose a reason for hiding this comment

Uh oh!

max-muoto Mar 5, 2023

Choose a reason for hiding this comment

Uh oh!

max-muoto Mar 5, 2023

Choose a reason for hiding this comment

Uh oh!

Nusnus Mar 5, 2023

Choose a reason for hiding this comment

Uh oh!

max-muoto Mar 5, 2023

Choose a reason for hiding this comment

Uh oh!

Nusnus Mar 5, 2023

Choose a reason for hiding this comment

Uh oh!

Nusnus commented Mar 5, 2023

Uh oh!

max-muoto commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Nusnus left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 5, 2023 •

edited

Loading

max-muoto commented Mar 8, 2023 •

edited

Loading