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

Add Application Emojis #2706

Closed

Conversation

Andre601
Copy link
Contributor

Pull Request Etiquette

Changes

  • Internal code
  • Library interface (affecting end-user code)
  • Documentation
  • Other: _____

Closes Issue: NaN

Description

Discord added the option to add emojis on a Bot's Dashboard for it to use globally without the requirement of being on a Guild for it.
This includes new routes for getting all application emojis, getting an application emoji by id, modifying one by id or deleting one by id.

Should we use the EntityBuilder for creating (Rich)CustomEmoji instances will this require modifications of the entire RichCustomEmoji setup, as they rely on a Guild to be present, which is not the case for application emojis right now, so it's probably best to make a new sub-type of the CustomEmoji, or rely on the static methods from it to create instances... if that makes sense.

PR is not yet in a finished state, hence draft.

(P.S. Ignore these two first commits. Somehow did they end up in the master branch from which I created this separate branch)

@freya022
Copy link
Contributor

freya022 commented Aug 5, 2024

I've added JDA#createApplicationEmoji(String, Icon) in freya022@abcf8d8 as it was missing, feel free to cherry-pick.

Dependency coordinates: io.github.freya022:JDA:abcf8d8219

@Andre601
Copy link
Contributor Author

Andre601 commented Aug 5, 2024

Could you PR this to my fork?
I'm not confident enough to use the cherry pick feature and don't want to risk messing my commits up.

@freya022
Copy link
Contributor

freya022 commented Aug 5, 2024

Could you PR this to my fork? I'm not confident enough to use the cherry pick feature and don't want to risk messing my commits up.

Done, I think you'll still have to fix the commits you added from the markdown stuff.

I'd recommend you create a branch from JDA's master, select all of the application emoji commits and cherry pick them all at once.

idea64_kXBprgjkmd.mp4

@Andre601
Copy link
Contributor Author

Andre601 commented Aug 5, 2024

The issue is, that these commits are in the main branch, so that's already an issue. I also can't see a main issue here with it tho, as the final PR can just squash or rebase commits on merge...

@freya022
Copy link
Contributor

freya022 commented Aug 5, 2024

The issue is, that these commits are in the main branch, so that's already an issue. I also can't see a main issue here with it tho, as the final PR can just squash or rebase commits on merge...

You can make a new branch from JDA's master branch, not necessarily yours. You can add the main JDA repo in your "Remotes". I recommend you "Checkout" the master branch from JDA, as it will also have the benefit of being updated with the main commits, instead of the ones from your branch. (meaning you no longer need to sync your fork)
image
image

However, make sure you don't try to push to a repo that isn't yours
image

It is an issue as PRs should mostly be limited to what they do, the markdown stuff here is completely unrelated to application emojis, and would also require even more time to make.

@Andre601
Copy link
Contributor Author

Andre601 commented Aug 5, 2024

It is an issue as PRs should mostly be limited to what they do, the markdown stuff here is completely unrelated to application emojis, and would also require even more time to make.

The markdown stuff isn't even in this PR. I reverted the changes made.

@Andre601 Andre601 mentioned this pull request Aug 5, 2024
6 tasks
@Andre601
Copy link
Contributor Author

Andre601 commented Aug 5, 2024

Superseded by #2712

@Andre601 Andre601 closed this Aug 5, 2024
@Andre601 Andre601 deleted the feature/add-application-emojis branch August 5, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants