add support for role icons #254

zeffo · 2021-10-07T09:40:56Z

Summary

This Pull Request adds support for Role Icons. This addresses #205.

Checklist

If code changes were made then they have been tested.
- I have updated the documentation to reflect the changes.
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, ...)

Dorukyum · 2021-10-07T09:56:38Z

Aren't role icons already in work in #224 🤔

izxxr · 2021-10-07T10:06:29Z

Aren't role icons already in work in #224 thinking

It needs improvements but I was and currently too busy so you can close 224 and merge this.

izxxr · 2021-10-07T10:08:02Z

Also @zeffo, unicode_emoji can also be set as role icon and is sent by discord so add that too.

Also, add icon and unicode_emoji to Role.edit()

Dorukyum · 2021-10-07T10:13:42Z

Yes, unicode_emoji is a data key on itself

zeffo · 2021-10-07T10:24:32Z

I didn't notice #224, my bad. Will make suggested changes, thank you both!

Lulalaby · 2021-10-07T10:45:11Z

I didn't notice #224, my bad. Will make suggested changes, thank you both!

224 is closed now. Continue here

discord/role.py

zeffo · 2021-10-07T11:40:25Z

@nerdguyahmad do the docstrings look okay?

discord/role.py

izxxr

Looks good 🚀

zeffo · 2021-10-07T11:48:34Z

isnt this exactly the same stuff which you did in your PR?

izxxr · 2021-10-07T11:53:56Z

isnt this exactly the same stuff which you did in your PR?

I didn't do unicode emojis.

zeffo · 2021-10-07T11:54:53Z

:| was just a few lines, you didnt have to close

izxxr · 2021-10-07T11:56:13Z

:| was just a few lines, you didnt have to close

I was too busy. besides, Swas pointed out that my PR was pointing to the feature/slash instead of master and I had to create a followup PR to fix that but I couldn't get time to do that.

Lulalaby · 2021-10-07T16:12:54Z

Pending check

discord/role.py

Lulalaby

Nothing works x~x

zeffo · 2021-10-07T17:57:51Z

that should fix it

Lulalaby

For now it's basically LGTM.

Add, Modify & Delete Icon/UnicodeEmoji is working.
However, I am afraid that we will have problems with deleting / removing the role icon when both was supplied.

We should probably set on a remove request both the icon and the unicode_emoji to None, to be sure that it'll get removed.

Implement a few logical checks.

Here is in example of DisCatSharp (C#)
Aiko-IT-Systems/DisCatSharp@459f12a

            if (emoji.HasValue && !iconb64.HasValue)
                pld.UnicodeEmoji = emoji;

            if (emoji.HasValue && iconb64.HasValue)
            {
                pld.IconBase64 = null;
                pld.UnicodeEmoji = emoji;
            }

            if (iconb64.HasValue)
                pld.IconBase64 = iconb64;

To be save look into our implementation commits: https://aitsys.dev/T31

Tested with https://aitsys.dev/F8400

izxxr · 2021-10-08T01:18:25Z

For now it's basically LGTM.

Add, Modify & Delete Icon/UnicodeEmoji is working. However, I am afraid that we will have problems with deleting / removing the role icon when both was supplied.

We should probably set on a remove request both the icon and the unicode_emoji to None, to be sure that it'll get removed.

Implement a few logical checks.

Here is in example of DisCatSharp (C#) Aiko-IT-Systems/DisCatSharp@459f12a
            if (emoji.HasValue && !iconb64.HasValue)
                pld.UnicodeEmoji = emoji;

            if (emoji.HasValue && iconb64.HasValue)
            {
                pld.IconBase64 = null;
                pld.UnicodeEmoji = emoji;
            }

            if (iconb64.HasValue)
                pld.IconBase64 = iconb64;
To be save look into our implementation commits: https://aitsys.dev/T31

Tested with https://aitsys.dev/F8400

I might have got you wrong but what about:

if unicode_emoji is not MISSING and icon is not MISSING:
  raise TypeError("unicode_emoji and icon keyword arguments cannot be mixed")

Lulalaby · 2021-10-08T06:45:58Z

I might have got you wrong but what about

True, well basically you can do it, but discord primary uses the icon then.
This restriction is up to the lib.

zeffo · 2021-10-08T06:52:50Z

From what I'm understanding, icon needs to be None if unicode_emoji is passed by the user, correct?

zeffo · 2021-10-08T07:06:53Z

I believe there are two ways we can go about this, one is to set icon to None if unicode_emoji is not MISSING, and the other way would be to check if unicode_emoji is not MISSING and icon is not None or MISSING and if so, raise an error. Which one sounds better?

Lulalaby · 2021-10-08T07:42:02Z

I would prefer to automatically set the icon to None and note it in the docs as stated here: #254 (review)

Lulalaby

LGTM

discord/role.py

CodeWithSwastik · 2021-10-08T13:54:11Z

LGTM 👍

* Prefer static_format over format with static assets * GitHub Action to lint Python code * bytes, not Bytes * Update lint_python.yml * Delete lint_python.yml * Fix typos discovered by codespell * Fix typos discovered by codespell * GitHub Action to run codespell * Make id parameter positional only * Code Of conduct is a suggestion by Github * Change `ctx.send` to `ctx.respond` in the README (#197) * Update README.rst * Update README.rst * Added START_EMBEDDED_ACTIVITIES permission #198 (#199) * Use start_embedded_activities instead of create_instant_invite * Use an Enum for embedded activites * Fix error in docstring * Add typehinting to pull request checklist * fix: distribution * change: display text * fixup! Fix typo for overridden * Changed unchanged change * Add *items to View * Add Pycord Development to LICENSE * License updates * Fix accidental deletion in 1257776 * Add dependabot * Add CodeQL * Bump sphinx from 4.0.2 to 4.2.0 Bumps [sphinx](https://github.com/sphinx-doc/sphinx) from 4.0.2 to 4.2.0. - [Release notes](https://github.com/sphinx-doc/sphinx/releases) - [Changelog](https://github.com/sphinx-doc/sphinx/blob/4.x/CHANGES) - [Commits](sphinx-doc/sphinx@v4.0.2...v4.2.0) --- updated-dependencies: - dependency-name: sphinx dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Set pycord user-agent (#241) * created delete_after parameter for ctx.respond (#180) * created delete_after parameter for ctx.respond * Update discord/interactions.py Co-authored-by: proguy914629 <74696067+proguy914629bot@users.noreply.github.com> * added eventloop thing * fixed ensure_future Co-authored-by: proguy914629 <74696067+proguy914629bot@users.noreply.github.com> * Added `on_raw_typing` event (#63) * Added `on_raw_typing` event * Fixes and adjustments for on_raw_typing Co-authored-by: Swas.py <61446939+CodeWithSwastik@users.noreply.github.com> * add support for role icons (#254) * add support for role icons * add unicode_emoji attribute and change Role.edit * role.edit required b64 string for icon * passing None should remove the icon * change documentation to reflect changes * update docstring for Role * invalid sentence * remove unnecessary documentation * add new valid keys * add valid keys * set icon to None if unicode_emoji is passed * Fix typo in docs * Use `ctx.respond` in favor of `ctx.send` * Use `ctx.respond` in favor of `ctx.send` * Added new embedded activites with documentation. (#267) * Added new embedded activities * Removed chess and renamed youtube2 * Documented `EmbeddedActivity` Enumeration * renamed youtube_advance to watch_together and added dev version of it Co-authored-by: Lala Sabathil <aiko@aitsys.dev> * Updated docs with the new naming convention Co-authored-by: Lala Sabathil <aiko@aitsys.dev> Co-authored-by: Lala Sabathil <aiko@aitsys.dev> * add support for sending file(s) in interaction response (#263) * add support for sending file(s) in interaction response * update docstring * fix * add ephemeral attribute to Attachment * use dict.get to avoid KeyErrors (#268) * Revert "Add a dunder len to Message (#176)" (#270) This reverts commit 2858bd1. * Add missing flags * Fix typo * Typos .-. * Fix typo Co-authored-by: Sonic4999 <sonictbm@gmail.com> Co-authored-by: Christian Clauss <cclauss@me.com> Co-authored-by: Izhar Ahmad <54180221+nerdguyahmad@users.noreply.github.com> Co-authored-by: PGamerX <websitedesigner123456@gmail.com> Co-authored-by: Gracie <87055757+Grace-codes@users.noreply.github.com> Co-authored-by: Middledot <78228142+Middledot@users.noreply.github.com> Co-authored-by: DeviousLab <deviouslab@gmail.com> Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com> Co-authored-by: Aomi Vel <69065737+AomiVel@users.noreply.github.com> Co-authored-by: BobDotCom <71356958+BobDotCom@users.noreply.github.com> Co-authored-by: Dorukyum <doruk.ak@hotmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ian Webster <ianw_github@ianww.com> Co-authored-by: Cheeseboy8020 <68305035+Cheeseboy8020@users.noreply.github.com> Co-authored-by: proguy914629 <74696067+proguy914629bot@users.noreply.github.com> Co-authored-by: Zeffo <43495198+zeffo@users.noreply.github.com> Co-authored-by: Prince Raj <68418241+Prince2347X@users.noreply.github.com> Co-authored-by: Lala Sabathil <aiko@aitsys.dev> Co-authored-by: Lala Sabathil <aiko@aiko-it-systems.eu>

Update documentation for #254

add support for role icons

89853c6

izxxr mentioned this pull request Oct 7, 2021

Add support for role icons #224

Closed

6 tasks

add unicode_emoji attribute and change Role.edit

d8f528f

izxxr suggested changes Oct 7, 2021

View reviewed changes

discord/role.py Outdated Show resolved Hide resolved

zeffo added 2 commits October 7, 2021 16:43

role.edit required b64 string for icon

7aa5d21

passing None should remove the icon

3babb83

izxxr suggested changes Oct 7, 2021

View reviewed changes

discord/role.py Outdated Show resolved Hide resolved

discord/role.py Show resolved Hide resolved

zeffo added 2 commits October 7, 2021 17:06

change documentation to reflect changes

de5987c

update docstring for Role

718aff0

invalid sentence

5e42505

izxxr reviewed Oct 7, 2021

View reviewed changes

discord/role.py Outdated Show resolved Hide resolved

remove unnecessary documentation

f028354

izxxr approved these changes Oct 7, 2021

View reviewed changes

Lulalaby reviewed Oct 7, 2021

View reviewed changes

discord/role.py Show resolved Hide resolved

Lulalaby requested changes Oct 7, 2021

View reviewed changes

zeffo added 2 commits October 7, 2021 23:23

add new valid keys

146e1f4

add valid keys

79bd6a7

Lulalaby requested changes Oct 7, 2021

View reviewed changes

set icon to None if unicode_emoji is passed

e387dc9

Lulalaby approved these changes Oct 8, 2021

View reviewed changes

Dorukyum reviewed Oct 8, 2021

View reviewed changes

discord/role.py Show resolved Hide resolved

CodeWithSwastik approved these changes Oct 8, 2021

View reviewed changes

CodeWithSwastik merged commit 26f422b into Pycord-Development:master Oct 8, 2021

BobDotCom added a commit to BobDotCom/pycord that referenced this pull request Oct 18, 2021

Update documentation for Pycord-Development#254

b172c40

BobDotCom mentioned this pull request Oct 18, 2021

Update documentation for #254 #281

Merged

6 tasks

BobDotCom added a commit that referenced this pull request Oct 19, 2021

Merge pull request #281 from BobDotCom/master

a6dff69

Update documentation for #254

Uh oh!

add support for role icons #254

add support for role icons #254

Uh oh!

Conversation

zeffo commented Oct 7, 2021

Summary

Checklist

Uh oh!

Dorukyum commented Oct 7, 2021

Uh oh!

izxxr commented Oct 7, 2021

Uh oh!

izxxr commented Oct 7, 2021

Uh oh!

Dorukyum commented Oct 7, 2021

Uh oh!

zeffo commented Oct 7, 2021

Uh oh!

Lulalaby commented Oct 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeffo commented Oct 7, 2021

Uh oh!

Uh oh!

izxxr left a comment

Choose a reason for hiding this comment

Uh oh!

zeffo commented Oct 7, 2021

Uh oh!

izxxr commented Oct 7, 2021

Uh oh!

zeffo commented Oct 7, 2021

Uh oh!

izxxr commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lulalaby commented Oct 7, 2021

Uh oh!

Uh oh!

Lulalaby left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeffo commented Oct 7, 2021

Uh oh!

Lulalaby left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

izxxr commented Oct 8, 2021

Uh oh!

Lulalaby commented Oct 8, 2021

Uh oh!

zeffo commented Oct 8, 2021

Uh oh!

zeffo commented Oct 8, 2021

Uh oh!

Lulalaby commented Oct 8, 2021

Uh oh!

Lulalaby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CodeWithSwastik commented Oct 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

izxxr commented Oct 7, 2021 •

edited

Loading

Lulalaby left a comment •

edited

Loading

Lulalaby left a comment •

edited

Loading