Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: SlashCommandGroup.walk_commands() error #1838

Merged
merged 2 commits into from Feb 15, 2023
Merged

fix: SlashCommandGroup.walk_commands() error #1838

merged 2 commits into from Feb 15, 2023

Conversation

krishnans2006
Copy link
Contributor

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

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

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
Member

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

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

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 enabled auto-merge (squash) February 14, 2023 18:47
plun1331
plun1331 requested changes Feb 14, 2023
View reviewed changes
discord/commands/core.py Outdated Show resolved Hide resolved
@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 merged commit 7789cbb into Pycord-Development:master Feb 15, 2023
@JustaSqu1d
Copy link
Member

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.

None yet
6 participants
@krishnans2006 @BobDotCom @Middledot @JustaSqu1d @Lulalaby @plun1331