Skip to content

fix: SlashCommandGroup.walk_commands() error#1838

Merged
Lulalaby merged 2 commits intoPycord-Development:masterfrom
krishnans2006:master
Feb 15, 2023
Merged

fix: SlashCommandGroup.walk_commands() error#1838
Lulalaby merged 2 commits intoPycord-Development:masterfrom
krishnans2006:master

Conversation

@krishnans2006
Copy link
Contributor

@krishnans2006 krishnans2006 commented Dec 23, 2022 •
edited by plun1331
Loading

Summary

Stops SlashCommandGroup.walk_commands() from returning SlashCommandGroups.

When SlashCommandGroups have other SlashCommandGroups as subcommands, they are also yielded in SlashCommandGroup.walk_commands(), even though only SlashCommands should be yielded from walk_commands().

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.

@krishnans2006 krishnans2006 requested a review from a team as a code owner December 23, 2022 21:57
@codecov
Copy link

codecov bot commented Dec 23, 2022 •
edited
Loading

Codecov Report

Merging #1838 (db72efc) into master (3b762f9) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1838   +/-   ##
=======================================
  Coverage   33.38%   33.38%           
=======================================
  Files          97       97           
  Lines       18815    18815           
=======================================
  Hits         6282     6282           
  Misses      12533    12533
Flag Coverage Δ
macos-latest-3.10 33.37% <100.00%> (ø)
macos-latest-3.11 33.37% <100.00%> (ø)
macos-latest-3.8 33.38% <100.00%> (ø)
macos-latest-3.9 33.38% <100.00%> (ø)
ubuntu-latest-3.10 33.37% <100.00%> (ø)
ubuntu-latest-3.11 33.37% <100.00%> (ø)
ubuntu-latest-3.8 33.38% <100.00%> (ø)
ubuntu-latest-3.9 33.38% <100.00%> (ø)
windows-latest-3.10 33.37% <100.00%> (ø)
windows-latest-3.11 33.37% <100.00%> (ø)
windows-latest-3.8 33.38% <100.00%> (ø)
windows-latest-3.9 33.38% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
discord/commands/core.py 17.99% <100.00%> (ø)

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 3b762f9...db72efc. Read the comment docs.

@BobDotCom
Copy link
Contributor

BobDotCom commented Jan 5, 2023

I'm unsure about this one, are we sure that we only want it to return SlashCommand objects?

cc. @Dorukyum

@krishnans2006
Copy link
Contributor Author

krishnans2006 commented Jan 5, 2023

are we sure that we only want it to return SlashCommand objects?

Looking at the function's return type, it seemed so, which is why I made the pr... if we choose to keep existing functionality, we should at least change the typehint to reflect what the method does.

@JustaSqu1d JustaSqu1d added the priority: medium Medium Priority label Jan 9, 2023
@Middledot
Copy link
Member

Middledot commented Feb 14, 2023

I think walk_commands should also return SlashCommandGroups for consistency and access to the sub groups. We should just change the typehint

@krishnans2006
fix: Update typehints and docstrings of `SlashCommandGroup.walk_comma…
695fe99 
…nds()`

to now return both `SlashCommand`s and `SlashCommandGroup`s
Lulalaby
Lulalaby previously approved these changes Feb 14, 2023
View reviewed changes
@Lulalaby Lulalaby requested a review from Middledot February 14, 2023 18:46
@Lulalaby Lulalaby enabled auto-merge (squash) February 14, 2023 18:47
plun1331
plun1331 requested changes Feb 14, 2023
View reviewed changes
@plun1331 plun1331 added documentation Improvements or additions to documentation status: awaiting review Awaiting review from a maintainer labels Feb 14, 2023
@krishnans2006 @plun1331
Update discord/commands/core.py
db72efc 
Co-authored-by: plun1331 <49261529+plun1331@users.noreply.github.com>
Signed-off-by: Krishnan Shankar <krishnans2006@gmail.com>
auto-merge was automatically disabled February 14, 2023 21:16

Head branch was pushed to by a user without write access

@plun1331 plun1331 enabled auto-merge (squash) February 14, 2023 21:58
@Lulalaby Lulalaby disabled auto-merge February 15, 2023 00:36
@Lulalaby Lulalaby merged commit 7789cbb into Pycord-Development:master Feb 15, 2023
@JustaSqu1d
Copy link
Member

JustaSqu1d commented Feb 15, 2023

Bruh changelog entry tho.

BobDotCom pushed a commit that referenced this pull request Mar 20, 2023
@krishnans2006 @BobDotCom
fix: SlashCommandGroup.walk_commands() error (#1838)
c994e5d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lulalaby Lulalaby Lulalaby approved these changes

@plun1331 plun1331 plun1331 approved these changes

@Middledot Middledot Awaiting requested review from Middledot

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation priority: medium Medium Priority status: awaiting review Awaiting review from a maintainer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@krishnans2006 @BobDotCom @Middledot @JustaSqu1d @Lulalaby @plun1331