fix(ext.commands): required is False although default is None

[solaluset]

Summary

#2089 (comment)

Information

• This PR fixes an issue.
• This PR adds something new (e.g. new method or parameters).
• This PR is a breaking change (e.g. methods or parameters removed/renamed).
• This PR is not a code change (e.g. documentation, README, typehinting,
  examples, ...).

Checklist

• I have searched the open pull requests for duplicates.
• If code changes were made then they have been tested.
  • I have updated the documentation to reflect the changes.
• If type: ignore comments were used, a comment is also left explaining why.
• I have updated the changelog to include these changes.

[Dorukyum]

I don't understand, there's no way x or y returns False when one side is True

[Lulalaby]

Yeah same thought here. Closing it as invalid.

[solaluset]

@Lulalaby @Dorukyum is this code valid?

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

I think it should work but it doesn't because the default is None.

UPDATE: BTW, it works as a slash command, but not as prefix command. Hence the inconsistency.

[plun1331]

I don't understand, there's no way x or y returns False when one side is True

The issue seems to be that it's returning True when it logically shouldn't (required is False, however default is None)

default being None should not change the behavior of required

[Dorukyum]

default being None should not change the behavior of required

Isn't that how it's always been? Option(...) would be required while Option(..., default=None) would be optional.

[solaluset]

default being None should not change the behavior of required

Isn't that how it's always been? Option(...) would be required while Option(..., default=None) would be optional.

Yes, but it didn't (and still doesn't) work for bridge commands invoked via prefix.

This PR fixes the issue.

[plun1331]

default being None should not change the behavior of required

Isn't that how it's always been? Option(...) would be required while Option(..., default=None) would be optional.

But if required is False with default being None, it automatically flips required to True (at least, that's my understanding of this pr)

[solaluset]

But if required is False with default being None, it automatically flips required to True (at least, that's my understanding of this pr)

Yes. Because of this the command above doesn't work without the argument.
The most confusing part is that it works when invoked as a slash command.

By the way, looks like Option already sets its .required attribute properly. Maybe it should be just required = param.annotation.required.

[solaluset]

Missclick 😅

[Dorukyum]

By the way, looks like Option already sets its .required attribute properly. Maybe it should be just required = param.annotation.required.

My bad, I understand the pr now and I think you're right. We can't do and default is None because default doesn't have to be None. Like you said, Option already sets required meaning we can just use required = param.annotation.required here.

.