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.

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

feat: Add image parameter to create_scheduled_event #1831

Conversation

Snawe
Copy link
Contributor

@Snawe Snawe commented Dec 18, 2022

Summary

The parameter cover was missing in the create_scheduled_event() function

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.

The parameter was missing at the create command
@Snawe Snawe requested a review from a team as a code owner December 18, 2022 10:57
@Snawe
Copy link
Contributor Author

Snawe commented Dec 18, 2022

during fixing this issue, I saw that the payload parameter is actually called image and not cover... Would it make sense to rename cover at the create and edit command to image? (But that would be a breaking change!)

@codecov
Copy link

codecov bot commented Dec 18, 2022

Codecov Report

Merging #1831 (14e5bfd) into master (b612eb0) will increase coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 14e5bfd differs from pull request most recent head 4b5e20a. Consider uploading reports for the commit 4b5e20a to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1831      +/-   ##
==========================================
+ Coverage   33.19%   33.20%   +0.01%     
==========================================
  Files          96       96              
  Lines       18670    18659      -11     
==========================================
- Hits         6197     6196       -1     
+ Misses      12473    12463      -10     
Flag Coverage Δ
macos-latest-3.10 33.19% <0.00%> (+0.01%) ⬆️
macos-latest-3.11 33.19% <0.00%> (+0.01%) ⬆️
macos-latest-3.8 33.20% <0.00%> (+0.01%) ⬆️
macos-latest-3.9 33.20% <0.00%> (+0.01%) ⬆️
ubuntu-latest-3.10 33.19% <0.00%> (+0.01%) ⬆️
ubuntu-latest-3.11 33.19% <0.00%> (+0.01%) ⬆️
ubuntu-latest-3.8 33.20% <0.00%> (+0.01%) ⬆️
ubuntu-latest-3.9 33.20% <0.00%> (+0.01%) ⬆️
windows-latest-3.10 33.19% <0.00%> (+0.01%) ⬆️
windows-latest-3.11 33.19% <0.00%> (+0.01%) ⬆️
windows-latest-3.8 33.20% <0.00%> (+0.01%) ⬆️
windows-latest-3.9 33.20% <0.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
discord/guild.py 23.77% <0.00%> (-0.06%) ⬇️
discord/http.py 22.98% <ø> (ø)
discord/iterators.py 16.04% <0.00%> (-0.12%) ⬇️
discord/enums.py 78.71% <0.00%> (-0.04%) ⬇️
discord/raw_models.py 28.28% <0.00%> (+0.18%) ⬆️
discord/automod.py 31.75% <0.00%> (+2.92%) ⬆️

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 b612eb0...4b5e20a. Read the comment docs.

@Dorukyum
Copy link
Member

during fixing this issue, I saw that the payload parameter is actually called image and not cover... Would it make sense to rename cover at the create and edit command to image? (But that would be a breaking change!)

The API wrapper should reflect the API, meaning the parameter should be called image.

@Snawe
Copy link
Contributor Author

Snawe commented Dec 19, 2022

during fixing this issue, I saw that the payload parameter is actually called image and not cover... Would it make sense to rename cover at the create and edit command to image? (But that would be a breaking change!)

The API wrapper should reflect the API, meaning the parameter should be called image.

Ok. Will change that as well. Should I attach the commit to this PR? Or should I create a new PR since it is a breaking change?

discord/guild.py Outdated Show resolved Hide resolved
discord/guild.py Outdated Show resolved Hide resolved
discord/guild.py Outdated Show resolved Hide resolved
discord/guild.py Outdated Show resolved Hide resolved
@Lulalaby
Copy link
Member

It's not a breaking change

@Snawe
Copy link
Contributor Author

Snawe commented Dec 19, 2022

It's not a breaking change

Why is it not a breaking change? You would need to change the paramter of edit here from cover to image.
edit does already exist. Changing a parameter in an existing function is a breaking change, isn't it?

For create I am with you. Since the parameter is new there, it doesn't matter.

@Lulalaby
Copy link
Member

oh didn't knew we have it there alr

We'll target that in another pr.
But only on the next breaking release.
So don't worry about it, we add this to our todo.
You can create a issue tho for that, that would help us to keep track.

Lulalaby
Lulalaby previously approved these changes Dec 19, 2022
@Lulalaby Lulalaby enabled auto-merge (squash) December 19, 2022 14:13
@Snawe
Copy link
Contributor Author

Snawe commented Dec 19, 2022

Will create an issue and attach a PR to it with the breaking changes.

BobDotCom
BobDotCom previously approved these changes Jan 5, 2023
@BobDotCom BobDotCom changed the title feat: Add cover parameter to create_scheduled_event feat: Add image parameter to create_scheduled_event Jan 5, 2023
@BobDotCom BobDotCom dismissed stale reviews from Lulalaby and themself via cf7c7a2 January 5, 2023 17:14
@Lulalaby Lulalaby disabled auto-merge January 5, 2023 17:18
@Lulalaby Lulalaby enabled auto-merge (squash) January 5, 2023 17:21
@Lulalaby Lulalaby merged commit b991975 into Pycord-Development:master Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants