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(Emoji): create getURL() function to get more extension of emoji #8493

Closed
wants to merge 24 commits into from
Closed

Conversation

dager-mohamed
Copy link
Contributor

@dager-mohamed dager-mohamed commented Aug 15, 2022

Please describe the changes this PR makes and why it should be merged:

discordjs support only .gif and .png for emoji extension, so I created the getURL(ext) function to make the user get any extension of emoji he wants.

Status and versioning classification:

Please move lines that apply to you out of the comment:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)

@vercel
Copy link

vercel bot commented Aug 15, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
discord-js ⬜️ Ignored (Inspect) Aug 15, 2022 at 3:25PM (UTC)

Copy link
Contributor

@manuelvleeuwen manuelvleeuwen left a comment

Choose a reason for hiding this comment

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

I'm not so sure about this one. This breaks consistency with other url getters in the library, and if users want to have another extension, they can use client.rest.cdn.emoji(emoji.id, 'jpg') for example. This function is a shortcut for that, but it breaks consistency, so I don't think it should be added like this.

packages/discord.js/src/structures/Emoji.js Outdated Show resolved Hide resolved
@jaw0r3k
Copy link
Contributor

jaw0r3k commented Aug 15, 2022

I agree with thehackerboi that .url should be a getter
So making function like getUrl would be better

@dager-mohamed
Copy link
Contributor Author

dager-mohamed commented Aug 15, 2022

I agree with thehackerboi that .url should be a getter So making a function like getUrl would be better

so I have an idea and I don't know if this idea is like you said or not,

I can create getUrl(URL) as you said function inside the class to check the URL of the emoji and then return the extension,
and I will use the getUrl() function inside url() and it will be getter that you mean?

I hope that you mean.

@jaw0r3k
Copy link
Contributor

jaw0r3k commented Aug 15, 2022

Nah i just mean getURL(ext) which will return a url
and leave url as getter ( like everywhere else )

@dager-mohamed
Copy link
Contributor Author

ahaa ok i understand now, i will do it

dager-mohamed and others added 3 commits August 15, 2022 16:44
Co-authored-by: Hackerboi 69 <62872992+thehackerboi69github@users.noreply.github.com>
@dager-mohamed dager-mohamed requested review from Syjalo and manuelvleeuwen and removed request for Syjalo August 15, 2022 15:25
@dager-mohamed
Copy link
Contributor Author

note: the rest.cdn is automatically throw the error at all

@codecov
Copy link

codecov bot commented Aug 15, 2022

Codecov Report

Merging #8493 (d0aaca6) into main (cb85686) will decrease coverage by 13.45%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##              main    #8493       +/-   ##
============================================
- Coverage   100.00%   86.54%   -13.46%     
============================================
  Files            2       87       +85     
  Lines          843     9250     +8407     
  Branches       159     1854     +1695     
============================================
+ Hits           843     8005     +7162     
- Misses           0     1189     +1189     
- Partials         0       56       +56     
Flag Coverage Δ
builders 100.00% <ø> (?)
collection 100.00% <ø> (ø)
proxy 83.45% <ø> (?)
rest 94.90% <ø> (?)
utilities 100.00% <ø> (ø)
voice 64.29% <50.00%> (?)
ws 63.60% <ø> (?)

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

Impacted Files Coverage Δ
packages/voice/src/audio/AudioPlayerError.ts 0.00% <0.00%> (ø)
packages/voice/src/audio/AudioResource.ts 95.60% <ø> (ø)
packages/voice/src/joinVoiceChannel.ts 100.00% <ø> (ø)
packages/voice/src/networking/Networking.ts 3.42% <ø> (ø)
packages/voice/src/VoiceConnection.ts 81.14% <100.00%> (ø)
packages/actions/src/formatTag/formatTag.ts 100.00% <0.00%> (ø)
packages/voice/src/receive/SSRCMap.ts 86.66% <0.00%> (ø)
...src/interactions/contextMenuCommands/Assertions.ts 100.00% <0.00%> (ø)
packages/voice/src/receive/SpeakingMap.ts 100.00% <0.00%> (ø)
... and 81 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Jiralite Jiralite added this to the discord.js v14.3 milestone Aug 15, 2022
@dager-mohamed dager-mohamed closed this by deleting the head repository Aug 27, 2022
@dager-mohamed dager-mohamed reopened this Sep 4, 2022
@dager-mohamed dager-mohamed changed the title feat(Emoji): support more extensions inside url() function feat(Emoji): create getURL() function to get more extension of emoji Sep 26, 2022
Comment on lines +59 to +66
/**
* @type {?string}
* @param {ImageExtension} ext the extension of emoji
* @returns {string}
*/
getURL(ext) {
return this.id && this.client.rest.cdn.emoji(this.id, ext);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i think the position of this function needs to be below all of the getter methods

Suggested change
/**
* @type {?string}
* @param {ImageExtension} ext the extension of emoji
* @returns {string}
*/
getURL(ext) {
return this.id && this.client.rest.cdn.emoji(this.id, ext);
}
/**
* @param {ImageExtension} ext the extension of emoji
* @returns {?string}
*/
getURL(ext) {
return this.id && this.client.rest.cdn.emoji(this.id, ext);
}

also corrected documentation

Copy link
Contributor

@jaw0r3k jaw0r3k Jul 30, 2023

Choose a reason for hiding this comment

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

Suggested change
/**
* @type {?string}
* @param {ImageExtension} ext the extension of emoji
* @returns {string}
*/
getURL(ext) {
return this.id && this.client.rest.cdn.emoji(this.id, ext);
}
/**
* Returns an emoji url with the specified extension
* @param {ImageExtension} ext The extension of the emoji
* @returns {?string}
*/
getURL(ext) {
return this.id && this.client.rest.cdn.emoji(this.id, ext);
}

@@ -1070,6 +1070,7 @@ export class Emoji extends Base {
public name: string | null;
public get identifier(): string;
public get url(): string | null;
public getURL(ext: ImageExtension): string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public getURL(ext: ImageExtension): string;
public getURL(ext: ImageExtension): string | null;

as per previous suggestion the method can return null

@Jiralite Jiralite removed this from the discord.js v14.7 milestone Nov 3, 2022
@Jiralite
Copy link
Member

Superseded by #9788.

@Jiralite Jiralite closed this Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants