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: change default attribute of SlashCommandGroup #2303

Merged
merged 10 commits into from
Jan 3, 2024
Merged

fix: change default attribute of SlashCommandGroup #2303

merged 10 commits into from
Jan 3, 2024

Conversation

Dasupergrasskakjd
Copy link
Contributor

@Dasupergrasskakjd Dasupergrasskakjd commented Dec 26, 2023 •
edited
Loading

Summary

Fixes #2302

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.
  • I have updated the changelog to include these changes.

@Dasupergrasskakjd Dasupergrasskakjd requested a review from a team as a code owner December 26, 2023 01:20
@pullapprove4 pullapprove4 bot requested a review from Lulalaby December 26, 2023 01:20
@Dasupergrasskakjd Dasupergrasskakjd changed the title Update core.py and changelog Fix #2302 Dec 26, 2023
@Dasupergrasskakjd Dasupergrasskakjd changed the title Fix #2302 fix: #2302 Dec 26, 2023
@Dorukyum
Copy link
Member

Couldn't we just replace if self.cog is not None: lines with if self.cog:?

@Lulalaby
Copy link
Member

Also please stick to naming conventions of pull requests.
fix: #number is not enough

@Dasupergrasskakjd Dasupergrasskakjd changed the title fix: #2302 fix: change checking for ApplicationCommand cog attribute Dec 26, 2023
@Dorukyum Dorukyum self-requested a review December 27, 2023 17:38
@Dasupergrasskakjd
Merge branch 'master' into fix-2302
aaf4028 
Signed-off-by: Dasupergrasskakjd <106623583+Dasupergrasskakjd@users.noreply.github.com>
@Dorukyum
Copy link
Member

I reviewed some code and decided there's no reason for the cog attribute to ever be MISSING. Would you like to modify this pull request to make that attribute default to None instead?

@Dasupergrasskakjd
Copy link
Contributor Author

I reviewed some code and decided there's no reason for the cog attribute to ever be MISSING. Would you like to modify this pull request to make that attribute default to None instead?

I'm also not sure about why would it be MISSING, but when I run the code described in #2302 it does raise that error

@Dorukyum
Copy link
Member

Dorukyum commented Jan 2, 2024

You got me wrong, I understand that you get that error because the attribute defaults to MISSING and that's what the code is supposed to do. I just don't see the point in that anymore so I think we should replace the default MISSING value with None.

@Dasupergrasskakjd
Copy link
Contributor Author

Dasupergrasskakjd commented Jan 2, 2024 •
edited
Loading

You got me wrong, I understand that you get that error because the attribute defaults to MISSING and that's what the code is supposed to do. I just don't see the point in that anymore so I think we should replace the default MISSING value with None.

Yes sorry

@Dasupergrasskakjd Dasupergrasskakjd changed the title fix: change checking for ApplicationCommand cog attribute fix: change default attribute of SlashCommandGroup Jan 2, 2024
@Dorukyum
Copy link
Member

Dorukyum commented Jan 3, 2024

Thanks, I'll finish this up.

@Dorukyum Dorukyum added bug Something isn't working priority: high High Priority status: in progress Work in Progess labels Jan 3, 2024
@Dorukyum
Copy link
Member

Dorukyum commented Jan 3, 2024

I've run the code provided in the issue and the bug seems to have been fixed, merging 👍

@Dorukyum Dorukyum enabled auto-merge (squash) January 3, 2024 16:38
@Dorukyum
Update CHANGELOG.md
2e31894 
Signed-off-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
Lulalaby
Lulalaby approved these changes Jan 3, 2024
View reviewed changes
Copy link
Member

@Lulalaby Lulalaby left a comment

Choose a reason for hiding this comment

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

meow meow

@Dorukyum Dorukyum merged commit fc7b104 into Pycord-Development:master Jan 3, 2024
29 checks passed
@ChunkLightTuna ChunkLightTuna mentioned this pull request Jan 6, 2024
Closed
9 tasks
OmLanke pushed a commit to OmLanke/pycord that referenced this pull request Jan 16, 2024
@Dasupergrasskakjd @Dorukyum @OmLanke
fix: change default attribute of SlashCommandGroup (Pycord-Developm…
3b73b19 
…ent#2303)

* Update core.py and changelog

* Update core.py

* Update core.py

* Update core.py

* chore: update changelog

* fix: remove all references to cog being MISSING

---------

Signed-off-by: Dasupergrasskakjd <106623583+Dasupergrasskakjd@users.noreply.github.com>
Co-authored-by: Dorukyum <doruk.ak@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dorukyum Dorukyum Dorukyum approved these changes

@Lulalaby Lulalaby Lulalaby approved these changes

Assignees

@Dorukyum Dorukyum

@Dasupergrasskakjd Dasupergrasskakjd

Labels
bug Something isn't working priority: high High Priority status: in progress Work in Progess
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Subgroup having MISSING cog attribute
3 participants
@Dasupergrasskakjd @Dorukyum @Lulalaby