decode message flags#718
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for exposing Discord message flags in Nostrum.Struct.Message and introduces a dedicated Nostrum.Struct.Message.Flags helper module to convert between the integer bitfield and a boolean-flag struct.
Changes:
- Introduce
Nostrum.Struct.Message.Flagswithfrom_integer/1andto_integer/1helpers. - Add a
:flagsfield (raw bitfield integer) toNostrum.Struct.Messageand attempt to cast it duringto_struct/1and update merges. - Add a message
flagstype to the struct typespecs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| lib/nostrum/struct/message/flags.ex | New helper struct/module for converting message flags between bitfield integers and booleans. |
| lib/nostrum/struct/message.ex | Adds :flags to the message struct and attempts to cast it during struct construction/updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jchristgit
left a comment
jchristgit
left a comment
There was a problem hiding this comment.
Thank you for the pull request!
Clanker comments are mostly fine, to be honest. I'm positively surprised. See my comments below.
We should think about whether we want the flags field in the Message to automatically be decoded. Right now, from my understanding, they are just being kept as-is.
lara-rium
left a comment
lara-rium
left a comment
There was a problem hiding this comment.
this should resolve all the comments, pls let me know if i need further adjustments
i just copied over from user, which also keeps it as it is. if we wanna discuss this i think we should do it in another issue cos it'd affect other flags as well. im also not very happy with this approach bc it involves copy/pasting the same |
jchristgit
left a comment
jchristgit
left a comment
There was a problem hiding this comment.
Thank you, this looks good!
I think we can remove the extra test cases from flags_test.exs because as far as I can tell they are the same as the doctests. What do you think?
Other than that, LGTM 
🦀
|
wdym by doctests? do the examples act as tests somehow? |
|
|
|
thats rly smart |
|
@lara-rium could you fix conflicts, please? Thank you! |
|
done |
| ``` | ||
| """ | ||
| @spec modify_member(Guild.id(), User.id() | :me, Api.options(), AuditLogEntry.reason()) :: | ||
| @spec modify_member(Guild.id(), User.id(), Api.options(), AuditLogEntry.reason()) :: |
There was a problem hiding this comment.
This looks to now be undoing some of your latest changes to master around the @me endpoint support, I assume this is unintentional and should get reverted.
There was a problem hiding this comment.
my thinking was that this pr is solely for decoding message flags so other features i added should be reverted. this conflict should probably be fixed while merging #717 or this pr. this wouldnt have happened if i hadnt forgotten to create a sole branch for #717 but it is what it is. lmk if u dont agree w this revert
There was a problem hiding this comment.
It would be best if you could drop these unrelated commits from the history of this branch, could you do that?
There was a problem hiding this comment.
hmm, how do i do that?
There was a problem hiding this comment.
So the basic idea would be something like this:
- Start by making a backup of your branch:
git checkout -b message-flags-backup, thengit checkout message-flags git rebase -i master- Do a local sanity check. If it's bad,
git reset --hard message-flags-backup git push -f
But I'm really struggling to see which commit actually intoduces the message flag decoding without other changes. It would maybe be best if you open a new pull request with only the message flag changes cherry picked out of here.
There was a problem hiding this comment.
i unfortunately don't have time for this right now.. anyone else is welcome to take this on
closes #711
i looked at user flags impl and reflected those on message but some things may be missing. pls lmk upon review. also sorry ab the messy revert commits, forgot to create different branches for each pr