Skip to content

fix: Updates type annotations to use Optional instead of union syntax#30

Merged
dorel14 merged 1 commit into
masterfrom
develop
May 25, 2026
Merged

fix: Updates type annotations to use Optional instead of union syntax#30
dorel14 merged 1 commit into
masterfrom
develop

Conversation

@dorel14

@dorel14 dorel14 commented May 25, 2026

Copy link
Copy Markdown
Owner

Changes type annotations for the scheduler parameter from union syntax (AsyncIOScheduler | None) to the Optional generic type for consistency across the codebase. Also adds Optional to the typing imports.

Changes type annotations for the scheduler parameter from union syntax (`AsyncIOScheduler | None`) to the `Optional` generic type for consistency across the codebase. Also adds `Optional` to the typing imports.
@dorel14 dorel14 merged commit 13e7bf3 into master May 25, 2026
34 of 35 checks passed

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request modifies taskiq_flow/scheduling/scheduler.py to replace the union type hint AsyncIOScheduler | None with Optional[AsyncIOScheduler]. The reviewer suggests using from __future__ import annotations instead, which allows keeping the cleaner AsyncIOScheduler | None syntax, avoids runtime errors when apscheduler is not installed, and maintains consistency with the rest of the codebase.

import logging
from datetime import datetime
from typing import Any, cast
from typing import Any, Optional, cast

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

By adding from __future__ import annotations at the very top of this file, we can avoid importing Optional entirely and keep the imports clean.

Suggested change
from typing import Any, Optional, cast
from typing import Any, cast

self,
broker: AsyncBroker,
scheduler: AsyncIOScheduler | None = None,
scheduler: Optional[AsyncIOScheduler] = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instead of switching to Optional[AsyncIOScheduler] and introducing inconsistency with the rest of the codebase (which consistently uses the | None union syntax, such as job_store_url: str | None on the next line), we can resolve the runtime TypeError (which occurs when apscheduler is not installed and AsyncIOScheduler is None) by adding from __future__ import annotations at the top of the file.

This postpones the evaluation of annotations, allowing us to safely use AsyncIOScheduler | None without runtime crashes.

Suggested change
scheduler: Optional[AsyncIOScheduler] = None,
scheduler: AsyncIOScheduler | None = None,

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.

1 participant