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

Emoji convenience methods #1378

Merged
merged 6 commits into from
Apr 30, 2017
Merged

Emoji convenience methods #1378

merged 6 commits into from
Apr 30, 2017

Conversation

PgBiel
Copy link
Contributor

@PgBiel PgBiel commented Apr 13, 2017

Adds convenience methods (similar to those of RichEmbed) to Emoji, for easier use:

  • setName
  • addRestrictedRole
  • addRestrictedRoles
  • removeRestrictedRole
  • removeRestrictedRoles

Still a bit indecisive over either stay using array argument or use rest parameters for the addRestrictedRoles method. Let me know if I should use rest parameters.

Edit: I can also use both 🤔
Edit 2: If I used both I could also merge the restricted role methods into one method 🤔

* @param {Role} role The role to add
* @returns {Promise<Emoji>}
*/
addRestrictedRole(role) {
Copy link
Member

Choose a reason for hiding this comment

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

this should probably just call this.addRestrictedRoles([role]) for consistancy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@devsnek
Copy link
Member

devsnek commented Apr 24, 2017

what about removing restricted roles

@PgBiel
Copy link
Contributor Author

PgBiel commented Apr 25, 2017

I'll see

for (const role of roles) {
if (newRoles.has(role.id)) newRoles.delete(role.id);
}
return newRoles;
Copy link
Member

Choose a reason for hiding this comment

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

return this.edit perhaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am wordless

@amishshah amishshah force-pushed the master branch 4 times, most recently from 260b09e to 5b2ca32 Compare April 29, 2017 18:45
PgBiel added 2 commits April 29, 2017 23:47
Requested by Crawl
@iCrawl iCrawl merged commit 9a5de25 into discordjs:master Apr 30, 2017
@iCrawl iCrawl added t: REST and removed t: utility labels May 1, 2017
devsnek pushed a commit to devsnek/discord.js that referenced this pull request May 14, 2017
* make branch

* stuff

* Consistency & remove role(s)

* kill me

* Doc changes

Requested by Crawl

* forgot 1
devsnek pushed a commit to devsnek/discord.js that referenced this pull request May 14, 2017
* make branch

* stuff

* Consistency & remove role(s)

* kill me

* Doc changes

Requested by Crawl

* forgot 1
Ratismal pushed a commit to Ratismal/discord.js that referenced this pull request Jul 5, 2017
* make branch

* stuff

* Consistency & remove role(s)

* kill me

* Doc changes

Requested by Crawl

* forgot 1
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.

3 participants