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 emoji argument #349

Merged
merged 9 commits into from
Jan 10, 2022
Merged

Conversation

feralheart
Copy link
Contributor

With this addition there will be a built-in argument for both global and guild emojis

src/arguments/CoreEmoji.ts Outdated Show resolved Hide resolved
src/arguments/CoreEmoji.ts Outdated Show resolved Hide resolved
src/arguments/CoreEmoji.ts Outdated Show resolved Hide resolved
@favna favna changed the title feature: Add emoji argument feature: add emoji argument Dec 28, 2021
favna added a commit to sapphiredev/website that referenced this pull request Dec 28, 2021
This should be merged alongside sapphiredev/framework#349
@favna favna changed the title feature: add emoji argument feat: add emoji argument Dec 28, 2021
favna
favna previously approved these changes Dec 28, 2021
src/arguments/CoreEmoji.ts Outdated Show resolved Hide resolved
src/lib/resolvers/emoji.ts Outdated Show resolved Hide resolved
@favna favna dismissed their stale review December 28, 2021 18:35

See vladdy's comments @feralheart

src/arguments/CoreEmoji.ts Outdated Show resolved Hide resolved
src/arguments/CoreEmoji.ts Outdated Show resolved Hide resolved
src/lib/resolvers/emoji.ts Outdated Show resolved Hide resolved
@feralheart feralheart requested a review from favna January 10, 2022 20:54
Copy link
Contributor

@noftaly noftaly left a comment

Choose a reason for hiding this comment

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

Maybe you should add a test for this resolver, it seems pretty easy.
With the following test cases: 😄, :smile:, smile, <:custom:737141877803057244>, <a:custom:737141877803057244> etc

Comment on lines 13 to 24
const emojiId = EmojiRegex.exec(parameter)?.groups?.id;

if (emojiId) {
const resolved = Util.parseEmoji(emojiId) as EmojiObject;

if (resolved) {
return ok(resolved);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is always going to return false for animated as per how parseEmoji works: https://github.com/discordjs/discord.js/blob/12ffa069aa8b247e945fef16a543f41c2c391bf1/packages/discord.js/src/util/Util.js#L284-L298

Instead pass the full parameter into the method.

image

Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

LGTM after this one thing

src/lib/parsers/Args.ts Outdated Show resolved Hide resolved
@favna favna merged commit 15f4e13 into sapphiredev:main Jan 10, 2022
favna added a commit to sapphiredev/website that referenced this pull request Jan 12, 2022
This should be merged alongside sapphiredev/framework#349
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

5 participants