-
-
Notifications
You must be signed in to change notification settings - Fork 482
feat: implement positional flags #2443
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Dorukyum <[email protected]> Co-authored-by: JustaSqu1d <[email protected]> Signed-off-by: Lala Sabathil <[email protected]>
@Vioshim please work on dorus comment and resolve conflcts |
Signed-off-by: Lala Sabathil <[email protected]>
The typing import in flags.py has been updated to include the Optional module. This change ensures that the __commands_flag_positional__ attribute can accept a value of None.
Signed-off-by: Dorukyum <[email protected]>
@Pycord-Development/contributors can we get some testing here :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File "C:\Users\Jérémie\Documents\GitHub\pycord\thing.py", line 23, in <module>
class BasicText(FlagConverter, prefix="--", delimiter=" "):
File "C:\Users\Jérémie\Documents\GitHub\pycord\thing.py", line 24, in BasicText
text: str = flag(positional=True)
^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\Jérémie\Documents\GitHub\pycord\discord\ext\commands\flags.py", line 143, in flag
return Flag(
^^^^^
TypeError: Flag.__init__() got an unexpected keyword argument 'name'
The error does not seem to be caused by this PR, but rather by me using python 3.11
. The error is not present in my testing when using python 3.10
.
fix in #2759
Other than that, this seems to work to me, although I don't think I'm experienced enough with flags - and prefixed commands to assess with good certainty that everything works as expected.
does this need testing or is it ready to merge ? |
Needs testing + doesn't work with python 3.11+ |
Merge conflicts |
@Lulalaby I think that OP is gone but I can fix these in a new pr. Lmk if ok |
Sure go for it if you still think it's a good addition |
My apologies, felt lost on how to be able to contribute in the project once i saw the things about merging conflicts or the documentation changes, so ended up lost on how to correct it alongside irl stuff, so my apologies for the lack of response |
@Vioshim no worries. Do you still want to work on this ? If lmk and I'll handle the merge conflicts and the rest |
That'd be quite appreciated, thank you!! |
No worries and thank you for your contribution ! |
Signed-off-by: Paillat <[email protected]>
@Lumabots Can I get some testing from you ? Specifically on python 3.11 and higher ? |
Signed-off-by: Paillat <[email protected]>
Yep doing that tomorrow evening |
b55c125
to
82659b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm code wise, if this is tested and works then should be ready to merge
Summary
This allows FlagConverters to be used after the first text expression, for example
Information
examples, ...).
Checklist
type: ignore
comments were used, a comment is also left explaining why.