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_id support for received messages #2335

Merged

Conversation

Almighty-Satan
Copy link
Contributor

Pull Request Etiquette

Changes

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

Closes Issue: #2311

Description

Adds Message#getApplicationId and Message#getApplicationIdLong to easily tell which bot sent the message without the need to look up webhooks.

Comment on lines 493 to 497
@Nonnull
default String getApplicationId()
{
return Long.toUnsignedString(getApplicationIdLong());
}
Copy link
Member

Choose a reason for hiding this comment

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

I feel like our pattern here is normally id or null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This matches the behavior of MessageChannel#getLatestMessageId, MessageReference#getMessageId, MessageReference#getChannelId and MessageReference#getGuildId. So I guess those should be changed as well? Maybe in a different PR?

Copy link
Member

Choose a reason for hiding this comment

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

MessageChannel#getLatestMessageId

This is older code from before we really landed on this pattern. We should update it.

MessageReference#getMessageId

A message will always have an Id. The docs on this method are wrong. It being "0" would be a bug.

MessageReference#getChannelId

A message will always have a channel. The docs on this method are wrong. It being "0" would be a bug.

MessageReference#getGuildId

This should be returning null if getGuildIdLong == 0, so yeah we need to update it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we explicitly decided to use "0" for getLatestMessageId.

Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

I just looked at the git history:
Originally this method threw an IllegalStateException when you called it if there wasn't a latestMessageId to use. We changed it to instead use String.valueOf(id) and later Long.toUnsignedString(id).

I don't see anything in the history that indicates to me that we chose to explicitly do "0"

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember where or when we talked about it, but I definitely remember discussing it when we removed the exception. And I don't really see a valid reason to change it now. Is there any example of where we do return null for missing id?

Copy link
Member

Choose a reason for hiding this comment

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

image
These at a minimum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MessageReference#getMessageId

A message will always have an Id. The docs on this method are wrong. It being "0" would be a bug.

MessageReference#getChannelId

A message will always have a channel. The docs on this method are wrong. It being "0" would be a bug.

According to the docs they are optional

Copy link
Member

Choose a reason for hiding this comment

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

Those docs are for sending, not receiving.

@MinnDevelopment MinnDevelopment merged commit d57a776 into discord-jda:master Nov 27, 2022
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

3 participants