Skip to content
This repository has been archived by the owner on Mar 13, 2023. It is now read-only.

feat: add hybrid commands #585

Merged
merged 45 commits into from
Aug 20, 2022
Merged

feat: add hybrid commands #585

merged 45 commits into from
Aug 20, 2022

Conversation

AstreaTSS
Copy link
Contributor

@AstreaTSS AstreaTSS commented Aug 2, 2022

What type of pull request is this?

  • Non-breaking code change
  • Breaking code change
  • Documentation change/addition
  • Tests change

Description

This PR adds hybrid commands, which allow using a slash command as a prefixed command too. Efforts have been taken to make sure the resulting prefixed command is as close to the slash command as possible.

This PR also, mostly by consequence:

  • Adds in HybridContext. An argument has been added to Client to allow setting a custom subclass of HybridContext, too.
  • Allows using custom signatures for prefixed commands. This is a very low level thing and should only be manipulated by those who know what they're doing.
  • Changes prefixed commands to base if it should use a kwarg for a parameter or not based on its parameter type instead of blindly guessing.
  • Adds kind to PrefixedCommandParameter.
  • Adds docs for hybrid commands.

Due to the many, many ways slash commands can be declared, there may be some bugs that I missed (though I did test a number of combinations and options).

Changes

See above. Usually, I would type something out, but I feel like this would be a repeat of the description if I bothered.

Checklist

  • I've formatted my code with Black
  • I've ensured my code works on Python 3.10.x
  • I've tested my code

This is different from every other one of the contexts in that
get_context is not where it is created, as it is a
wrapper for two other contexts instead.

This shouldn't really matter too much, though.
I'd figured this was safer than hoping they read the logger.
@silasary
Copy link
Collaborator

silasary commented Aug 3, 2022

Can you go into tests/test_protocols.py and a check that Hybrid contexts are SendableContexts?

@AstreaTSS
Copy link
Contributor Author

Done!

@LordOfPolls
Copy link
Member

Due to the scope of this PR it should not be merged until we have at least a second trust-worthy reviewer

That said if it hits 14 days since my review without a second, it'll be merged

@LordOfPolls LordOfPolls added New Feature::Library A new feature for the library Requires Review This PR requires additional reviews before it can be merged labels Aug 10, 2022
@AstreaTSS
Copy link
Contributor Author

Found a little oversight - min/max_length wasn't being respected. That's been fixed with the latest two commits.

@LordOfPolls LordOfPolls merged commit 1898b8d into NAFTeam:dev Aug 20, 2022
@AstreaTSS AstreaTSS deleted the hybrid-cmds branch November 24, 2022 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
New Feature::Library A new feature for the library Requires Review This PR requires additional reviews before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants