procrastinate-org / procrastinate

PostgreSQL-based Task Queue for Python
https://procrastinate.readthedocs.io/
MIT License
800 stars 52 forks source link

improve decorators type annotations #1061

Closed onlyann closed 1 month ago

onlyann commented 2 months ago

Closes #1056

Successful PR Checklist:

PR label(s):

This improves typing of the @App.task and @App.periodic decorators.

basic task

The @task decorator now returns a generic Task that defines the func and defer parameters.

For example, given the task below:

@app.task
async def sum(a: int, b: int):
    return a + b

sum is:

(function) sum: Task[(a: int, b: int), (a: int, b: int)]

[(a: int, b: int) describes the parameters of the wrapped func while (a: int, b: int) describes the parameters of the defer_async and defer functions.

⚠️ The Task signature doesn't enforce keyword arguments. Specifying positional arguments when calling defer will likely fail but won't be caught by type check. One way to improve type safety is to prevent positional arguments on the decorated function .e.g. async def sum(*, a: int, b: int)

task with context

Another example is the use of the passContext option:

@app.task(pass_context= True)
async def sum(context: JobContext, a: int, b: int):
    return a + b

sum is:

(function) sum: Task[(JobContext, a: int, b: int), (a: int, b: int)]

This is because JobContext is injected by the worker but should not be part of the defer arguments. The type annotation also correctly complains if the decorated function is missing a first parameter of typeJobContext.

Periodic task

Yet another example is the periodic decorator:

@app.periodic(cron="0 * * * 1")
@app.task
async def cleanup(timestamp: int):
    pass

cleanup is

Task[(timestamp: int), ()]

This is because timestamp is injected by the decorator but should not be part of the defer arguments.

⚠️ timestamp is actually set as a keyword parameter while the type annotation enforces a positional parameter. Unfortunately, concatenating keywords parameters is not supported.

Examples demonstrating the limitations:

@app.periodic(cron="0 * * * 1")
@app.task
# This works at runtime but fails type annotation
async def cleanup(what: str, timestamp: int):
    pass
@app.periodic(cron="0 * * * 1")
@app.task
# This fails at runtime but type annotation has no error
async def cleanup(ts: int):
    pass

Other changes

Options supported by both the task and periodic decorators are fully defined.

Other comments

The task_kwargs keyword parameter from Task.configure remains of type JSONDict. I am unsure if this can be accurately typed.

I initially added another generic parameter for the task return type but eventually removed it as I didn't see much value.

I added type hints in some of the test files to demonstrate the type annotations. That said, given pyright is configured to ignore the tests folder, breaking type hints won't be enforced during CI. Including type check for all tests would have made this pull request much larger, if that is even something desirable. Maybe there is a middle ground where some test files are included. I lean on you to suggest the best course of action here.

github-actions[bot] commented 2 months ago

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  procrastinate
  blueprints.py
  periodic.py
  tasks.py
Project Total  

This report was generated by python-coverage-comment-action