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

refactor!: embed classes #2063

Merged
merged 17 commits into from May 10, 2023
Merged

refactor!: embed classes #2063

merged 17 commits into from May 10, 2023

Conversation

OmLanke
Copy link
Contributor

@OmLanke OmLanke commented May 9, 2023

Summary

Breaking Change -

Attributes for Author, Footer, Image, Thumbnail, Video, and Provider on the embed will now return None instead of EmbedProxy, when they are not set. This change is done for correct API reflection.

This allowed the complete removal of EmbedProxy in favour of separate classes for the above attributes. This allows for concrete typing support. The new classes are EmbedAuthor, EmbedFooter, EmbedMedia and EmbedProvider

Breaking Change -

Embed.Empty and EmptyEmbed have now been removed in favour of None

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.

@Lulalaby
Copy link
Member

Lulalaby commented May 9, 2023

we shouldn't set required fields to none.
i.e. author.name

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #2063 (952bf1a) into master (5175fae) will decrease coverage by 0.02%.
The diff coverage is 27.81%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2063      +/-   ##
==========================================
- Coverage   33.24%   33.22%   -0.02%     
==========================================
  Files          97       97              
  Lines       19039    19092      +53     
==========================================
+ Hits         6329     6343      +14     
- Misses      12710    12749      +39     
Flag Coverage Δ
macos-latest-3.10 33.20% <27.81%> (-0.02%) ⬇️
macos-latest-3.11 33.20% <27.81%> (-0.02%) ⬇️
macos-latest-3.8 33.21% <27.81%> (-0.02%) ⬇️
macos-latest-3.9 33.21% <27.81%> (-0.02%) ⬇️
ubuntu-latest-3.10 33.20% <27.81%> (-0.02%) ⬇️
ubuntu-latest-3.11 33.20% <27.81%> (-0.02%) ⬇️
ubuntu-latest-3.8 33.21% <27.81%> (-0.02%) ⬇️
ubuntu-latest-3.9 33.21% <27.81%> (-0.02%) ⬇️
windows-latest-3.10 33.20% <27.81%> (-0.02%) ⬇️
windows-latest-3.11 33.20% <27.81%> (-0.02%) ⬇️
windows-latest-3.8 33.21% <27.81%> (-0.02%) ⬇️
windows-latest-3.9 33.21% <27.81%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
discord/embeds.py 25.21% <27.81%> (+0.21%) ⬆️

Continue to review full report in Codecov by Sentry.

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

@plun1331 plun1331 linked an issue May 9, 2023 that may be closed by this pull request
3 tasks
@plun1331 plun1331 added bug Something isn't working status: in progress Work in Progess changelog needed labels May 9, 2023
@plun1331 plun1331 added this to the v2.5 milestone May 9, 2023
@plun1331 plun1331 added the priority: high High Priority label May 9, 2023
discord/embeds.py Outdated Show resolved Hide resolved
discord/embeds.py Outdated Show resolved Hide resolved
discord/embeds.py Outdated Show resolved Hide resolved
@Lulalaby
Copy link
Member

nice work om

@Lulalaby Lulalaby added API Reflection Discords API wasn't correctly reflected in the lib status: awaiting review Awaiting review from a maintainer and removed status: in progress Work in Progess labels May 10, 2023
Copy link
Member

@NeloBlivion NeloBlivion left a comment

Choose a reason for hiding this comment

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

All in all a good effort, mainly needs some fine tuning.
Please update all necessary docs;

  • attrs like author, footer and video should refer to their new class.
  • Remove Empty and EmbedProxy references.
  • Add all new classes such as EmbedMedia to data_classes.rst.

Any changes you make to docs can be previewed here.

discord/embeds.py Show resolved Hide resolved
discord/embeds.py Outdated Show resolved Hide resolved
discord/embeds.py Outdated Show resolved Hide resolved
discord/embeds.py Show resolved Hide resolved
@Lulalaby Lulalaby changed the title feat!: Embed Changes feat!: embed refactor May 10, 2023
@Lulalaby Lulalaby changed the title feat!: embed refactor refactor!: embed classes May 10, 2023
@OmLanke OmLanke marked this pull request as ready for review May 10, 2023 20:32
@OmLanke OmLanke requested a review from a team as a code owner May 10, 2023 20:32
Lulalaby
Lulalaby previously approved these changes May 10, 2023
@Lulalaby Lulalaby requested a review from NeloBlivion May 10, 2023 20:57
@Lulalaby Lulalaby removed the request for review from NeloBlivion May 10, 2023 21:01
@pullapprove4
Copy link

pullapprove4 bot commented May 10, 2023

This pull request is in the In review step of the Default workflow.

Since there are no pending review teams, @Om1609 shoulde determine what is required to move this PR forward.

@Lulalaby Lulalaby merged commit 23da5e3 into Pycord-Development:master May 10, 2023
25 checks passed
@OmLanke OmLanke deleted the embed branch May 11, 2023 03:53
@Revnoplex
Copy link
Contributor

Revnoplex commented May 17, 2023

Not sure if this has been noted as part of the breaking changes but I noticed another breaking change which was the fields argument of Embed is no longer accepting None
which would cause 'NoneType' object is not iterable here:

File "bot-dir/venv/lib/python3.11/site-packages/discord/embeds.py", line 995, in to_dict
    result["fields"] = [field.to_dict() for field in self._fields]
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@OmLanke
Copy link
Contributor Author

OmLanke commented May 17, 2023

Yes that seems like a missed bug. I'll fix this in a bit

@OmLanke
Copy link
Contributor Author

OmLanke commented May 18, 2023

Yes that seems like a missed bug. I'll fix this in a bit

Already fixed by #2071

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Reflection Discords API wasn't correctly reflected in the lib bug Something isn't working priority: high High Priority status: awaiting review Awaiting review from a maintainer
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

'Embed.author.name' positional argument
6 participants