search

procrastinate-org / procrastinate

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

fix: add default args to cli shell command to avoid missing args error #1095

Open linspw opened 1 week ago

linspw commented 1 week ago

Closes #

Hi! How is it going?

First, thank you so much for this project; it is amazing!

The problem that this PR proposes to resolve occurs when we try to use ./manage.py procrastinate shell in django. The command raises the following error:

2024-07-03 13:42:31,117 DEBUG   procrastinate.cli Exception details:
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/procrastinate/cli.py", line 522, in execute_command
    await parsed.pop("func")(app=app, **parsed)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: shell_() missing 1 required positional argument: 'args'
shell_() missing 1 required positional argument: 'args'

With that simple change, everthing works normally.

Successful PR Checklist:

PR label(s):

ducdetronquito commented 1 week ago

Hey @linspw !

I'm not a maintainer of procrastinate, but I would like to just give some feedbacks on your PR.

You should avoid to use mutable default argument in Python because they are persisted accross calls to the function and therefore can lead to very hard to debug bugs if this argument is mutated within the function.

An example:

def do_something(args: list[str] = []) -> None:
    args.append("Hello")
    print(args)

do_something()
# Output: ["Hello"]

do_something()
# Output: ["Hello", "Hello]

# Etc....

I would recommend to use None as a replacement to notify that no args are provided by the caller:

async def shell_(app: procrastinate.App, args: list[str] | None = None):
    # If `args` evaluate to `False` (by being `None` or an empty list), set `args` to be an empty list
    # We could use if/else, but I find `or` to be more concise for these type of checks
    args = args or []
    # Continue with the rest of the function

Have a nice day :)

medihack commented 1 week ago

@linspw Thanks a lot for your contribution. Even when we don't mutate args currently, I think @ducdetronquito is correct (one never knows what the future brings 😉). How about just changing the function definition to args: list[str] | None = None? (I don't think we need the args = args or [] in this case).

ewjoachim commented 1 week ago

Hm, upon closer inspection, we're treating a symptom, not the cause.

The issue is that args seems to not be passed all the way to the command, but only when used through Django's manage.py procrastinate, but even with a default value, ./manage.py procrasinate shell list_tasks won't work. We need to understand and solve that issue, I think.

Note: It's most certainly linked to #1038 which added args to procrastinate shell.

ewjoachim commented 1 week ago

Here's the reason: Django is removing parameters named "args" for compatibility with optparse. If our option was named anything else than args, it would work.

My suggestion for a fix is twofold: