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

Allow supressing embeds on Message #240

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

Quintasan
Copy link
Contributor

Summary

  • Add pry-byebug to development dependencies
  • Add API::Channel#suppress_embeds
  • Add Message#suppress_embeds

Copy link
Contributor

@Droid00000 Droid00000 left a comment

Choose a reason for hiding this comment

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

Mainly looks good just needs a few minor changes. The main thing is that I'd say the pry-byebug dependency isn't really needed and goes way beyond the scope of this PR.

lib/discordrb/data/message.rb Outdated Show resolved Hide resolved
discordrb.gemspec Outdated Show resolved Hide resolved
@Quintasan Quintasan force-pushed the 223-support-setting-message-flags branch from 4c23b70 to d1544ff Compare October 14, 2024 11:06
@Quintasan Quintasan requested a review from Droid00000 October 14, 2024 11:07
@Quintasan
Copy link
Contributor Author

Mainly looks good just needs a few minor changes. The main thing is that I'd say the pry-byebug dependency isn't really needed and goes way beyond the scope of this PR.

We should be good to go now

@Droid00000
Copy link
Contributor

Mainly looks good just needs a few minor changes. The main thing is that I'd say the pry-byebug dependency isn't really needed and goes way beyond the scope of this PR.

We should be good to go now

Almost there, I think you forgot to remove the dependency from the spec dependency though 😅

@Quintasan Quintasan force-pushed the 223-support-setting-message-flags branch from d1544ff to d8c3f3e Compare October 14, 2024 18:49
@Quintasan
Copy link
Contributor Author

Mainly looks good just needs a few minor changes. The main thing is that I'd say the pry-byebug dependency isn't really needed and goes way beyond the scope of this PR.

We should be good to go now

Almost there, I think you forgot to remove the dependency from the spec dependency though 😅

Derp, done

@Quintasan Quintasan force-pushed the 223-support-setting-message-flags branch from d8c3f3e to 77731ae Compare November 12, 2024 20:57
@Quintasan
Copy link
Contributor Author

@Droid00000 well, this should be good now

@wouterdedroog
Copy link
Contributor

Looks great! It could be interesting to use this PR to further implement message flags (in a similar way to how the permissions class is currently structured). This is what I came up with: https://gist.github.com/wouterdedroog/59ed3076558c6d943dca5f8c75d5437f

@Quintasan
Copy link
Contributor Author

@Droid00000 bump

@swarley
Copy link
Member

swarley commented Dec 10, 2024

Would you be open to doing the API portion through a message update rather than a dedicated API method? I don't want to add an API level method for every flag that may need to be updated. Just something like

flags = @flags | SUPPRESS_EMBEDS
API::Channel.edit_message(..., flags)

@Quintasan
Copy link
Contributor Author

Would you be open to doing the API portion through a message update rather than a dedicated API method? I don't want to add an API level method for every flag that may need to be updated. Just something like

flags = @flags | SUPPRESS_EMBEDS
API::Channel.edit_message(..., flags)

I most certainly don't mind, but I'd like to work on this incrementally. When this gets merged, then I'll prepare another PR

@Quintasan
Copy link
Contributor Author

@Droid00000 sorry if I'm annyoing but it would be nice to merge this so I can iterate on this

@swarley swarley merged commit ff9a68e into shardlab:main Jan 7, 2025
7 checks passed
@Quintasan
Copy link
Contributor Author

🎊 I'll try whipping something up to address #240 (comment) this weekend

@swarley
Copy link
Member

swarley commented Jan 7, 2025

I merged Droid's branch that has the feedback addressed using your branch as the base

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.

4 participants