Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent default arguments in bridge commands #2089

Closed
3 tasks done
solaluset opened this issue May 28, 2023 · 4 comments · Fixed by #2252 or #2256
Closed
3 tasks done

Inconsistent default arguments in bridge commands #2089

solaluset opened this issue May 28, 2023 · 4 comments · Fixed by #2252 or #2256
Labels
bug Something isn't working ext.bridge Relating to ext.bridge on hold

Comments

@solaluset
Copy link
Contributor

Summary

Bridge commands do not always handle default arguments in the same way

Reproduction Steps

  1. Create a bridge command with non-required argument (via Option kwargs)
  2. Try to run it without the argument in question

Minimal Reproducible Code

import os

import discord
from discord.ext import bridge
# Option MUST be imported after ext.bridge
# otherwise it won't work with bridge commands
from discord import Option


bot = bridge.Bot()


@bot.bridge_command()
async def test_default1(ctx, text: Option(str) = "hello"):
    await ctx.respond(text)


@bot.bridge_command()
async def test_default2(ctx, text: Option(str, default="hello")):
    await ctx.respond(text)


@bot.bridge_command()
async def test_no_default(ctx, text: Option(str, required=False)):
    await ctx.respond(text or "hello")


bot.run(os.getenv("TOKEN"))

Expected Results

I'd expect all three commands from the above code to produce the same output when the argument isn't provided, like it does for slash variants:
Screenshot_20230528-133333989 (1)

Actual Results

However, only the first one works:
Screenshot_20230528-133607047 (1)
Others are throwing an error:

Traceback (most recent call last):
  File "/data/data/com.termux/files/home/tests/lib/python3.11/site-packages/discord/ext/bridge/bot.py", line 143, in invoke
    await ctx.command.invoke(ctx)
  File "/data/data/com.termux/files/home/tests/lib/python3.11/site-packages/discord/ext/commands/core.py", line 942, in invoke
    await self.prepare(ctx)
  File "/data/data/com.termux/files/home/tests/lib/python3.11/site-packages/discord/ext/commands/core.py", line 872, in prepare
    await self._parse_arguments(ctx)
  File "/data/data/com.termux/files/home/tests/lib/python3.11/site-packages/discord/ext/commands/core.py", line 774, in _parse_arguments
    transformed = await self.transform(ctx, param)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/data/data/com.termux/files/home/tests/lib/python3.11/site-packages/discord/ext/bridge/core.py", line 105, in transform
    return await super().transform(ctx, param)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/data/data/com.termux/files/home/tests/lib/python3.11/site-packages/discord/ext/commands/core.py", line 601, in transform
    raise MissingRequiredArgument(param)
discord.ext.commands.errors.MissingRequiredArgument: text is a required argument that is missing.

Intents

None

System Information

  • Python v3.11.2-final
  • py-cord v2.4.1-final
  • aiohttp v3.8.4
  • system info: Linux 4.14.232-QuicksilveR™-ReloadedOS-Edition #1 SMP PREEMPT Wed May 26 07:08:17 UTC 2021

Checklist

  • I have searched the open issues for duplicates.
  • I have shown the entire traceback, if possible.
  • I have removed my token from display, if visible.

Additional Context

Not related to this particular issue, but I don't quite like how Option works with bridge commands. Relying on the order of imports is something very non-obvious, confusing and, iirc, not documented.

@solaluset solaluset added the unconfirmed bug A bug report that needs triaging label May 28, 2023
@JustaSqu1d JustaSqu1d added ext.bridge Relating to ext.bridge bug Something isn't working and removed unconfirmed bug A bug report that needs triaging labels May 31, 2023
@JustaSqu1d JustaSqu1d removed their assignment Jun 5, 2023
@Middledot
Copy link
Member

Not sure when but it was fixed, but testing your code seems to work fine on latest (927f8ce).
image

I also think modifying Option on import is a bit hacky, and the solution seems to just force people to use BridgeOption, so that's what I'll do.

@solaluset
Copy link
Contributor Author

@Middledot your screenshot shows slash invocation, have you tried prefix invocation?

Also, BridgeOption is not documented and can only be accessed as bridge.core.BridgeOption which suggests that it's part of internal API.

@Middledot
Copy link
Member

Thank you for pointing that out cuz I did not try that. Text commands don't process Option, so that's what it does now (#2256)

For BridgeOption, I think we should expose it as the option class bridge commands should use instead of Option (#2252) but I want to wait on some internal stuff to sort out before merging

Dorukyum added a commit that referenced this issue Nov 29, 2023
…2256)

* fix(ext.commands): command parsing bug

#2089

* changelog: changelog

* style(pre-commit): auto fixes from pre-commit.com hooks

* Apple code suggestion

Co-authored-by: Dorukyum <[email protected]>
Signed-off-by: Middledot <[email protected]>

---------

Signed-off-by: Middledot <[email protected]>
Signed-off-by: Dorukyum <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Dorukyum <[email protected]>
@solaluset
Copy link
Contributor Author

Excuse me, but test_no_default still doesn't work as a prefix command. According to my testing, here and is necessary instead of or:

required = param.annotation.required or default is None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ext.bridge Relating to ext.bridge on hold
Projects
None yet
4 participants