Skip to content
This repository was archived by the owner on Dec 26, 2022. It is now read-only.

Conversation

@zunda-arrow
Copy link
Member

@zunda-arrow zunda-arrow commented Jan 2, 2022

Changes

  • adds: Command groups, they probably work.

  • fixed: Maybe the first PR where I didn't have to fix anything 🚀
    lmao that was a lie. I had to switch the __ttl argument in HTTP to _ttl.
    Also fixed doc building issues.

  • improvements: None

I want to get a release out after this.

Check off the following

  • I have tested my changes with the current requirements (Does anyone know what the requirements are??? I've just been checking this off)
  • My Code follows the pep8 code style.

@codecov
Copy link

codecov bot commented Jan 2, 2022

Codecov Report

Merging #342 (aec6bc4) into main (255cc3a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #342   +/-   ##
=======================================
  Coverage   90.54%   90.54%           
=======================================
  Files           8        8           
  Lines          74       74           
=======================================
  Hits           67       67           
  Misses          7        7           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 255cc3a...aec6bc4. Read the comment docs.

@zunda-arrow zunda-arrow requested review from Enderchief, Sigmanificient and trag1c and removed request for Sigmanificient January 2, 2022 02:17
zunda-arrow and others added 2 commits January 2, 2022 15:03
@zunda-arrow
Copy link
Member Author

zunda-arrow commented Jan 3, 2022

Just found a major optimization in the built_register 🚀🚀🚀🚀🚀
I just changed some things in __build_local_commands to use AppCommands instead of ClientCommandStructure. Shouldn't be too important but you can look at it if you want.

Note to @trag1c: this change made your 2 last requested changes useless 💀


# NOTE: Cannot be generator since it can't be consumed due to lines 743-745
to_remove = list(filter(should_be_removed, self._api_commands))
to_remove = [*filter(should_be_removed, self._api_commands)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is star operator faster/better than list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is star operator faster/better than list?

I found it about 1-2% faster so probably no difference. Looks better though

Copy link
Member Author

@zunda-arrow zunda-arrow Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the list function is much more readable in this case

@Sigmanificient Sigmanificient added the enhancement New feature or request label Jan 3, 2022
@Enderchief
Copy link
Member

Just a suggestion (you don't have to do this now).
The group/subgroup class should have a method called command which does the same doing @command(group=somegroup)

ex:

class Bot:
    group = Group("cool_commands")
    @group.command
    async def a_very_cool_command():
        pass

Copy link
Member

@Enderchief Enderchief left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just found some docstring issues

@zunda-arrow
Copy link
Member Author

zunda-arrow commented Jan 7, 2022

Just a suggestion (you don't have to do this now). The group/subgroup class should have a method called command which does the same doing @command(group=somegroup)

ex:

class Bot:
    group = Group("cool_commands")
    @group.command
    async def a_very_cool_command():
        pass

This is extremely easy to do but I didn't do it because command has like a hundred params so there would a lot of code duplication. Its extremely easy to do though so I can do it if you guys think its fine.

edit: why did i write its extremely easy to do twice xD. 💀

ty for merging btw 🙏

@trag1c trag1c merged commit c407aa1 into Pincer-org:main Jan 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants