Compatibility changes for Discord.py#1572
Compatibility changes for Discord.py#1572whinis wants to merge 2 commits intospacebarchat:masterfrom
Conversation
|
When I get a chance I'll review this change |
|
Thats ok, I am using this as a draft to fix multiple issues with discord.py so going to try a few things first. |
621c153 to
d452006
Compare
|
Still breaking on https://github.com/Rapptz/discord.py/blob/master/discord/member.py#L331 (sorry, i was running a patched version of discord.py to bypass this, but this will break for others) |
MathMan05
left a comment
There was a problem hiding this comment.
https://docs.discord.food/resources/message#message-object
Please use the documentation to replicate the structure of the message object. I could be wrong in some of these spots.
| // the position field. Needs to be looked into more | ||
| // TODO: Look into when this can be undefined during CHANNEL_UPDATE | ||
| if (this.position === undefined) { | ||
| this.position = 0; |
There was a problem hiding this comment.
please do this in the object instead
| @Column({ nullable: true }) | ||
| @RelationId((message: Message) => message.channel) | ||
| @Index() | ||
| @JsonRemoveEmpty |
|
|
||
| @Column({ nullable: true }) | ||
| @RelationId((message: Message) => message.thread) | ||
| @JsonRemoveEmpty |
There was a problem hiding this comment.
This is not a property that should end up in the result
|
|
||
| @Column({ nullable: true }) | ||
| @RelationId((message: Message) => message.guild) | ||
| @JsonRemoveEmpty |
There was a problem hiding this comment.
This is not a property of the message object
| @ManyToOne(() => Guild, { | ||
| onDelete: "CASCADE", | ||
| }) | ||
| @JsonRemoveEmpty |
There was a problem hiding this comment.
This is not a property of the message object
| edited_timestamp?: Date; | ||
|
|
||
| @Column({ nullable: true }) | ||
| @JsonRemoveEmpty |
There was a problem hiding this comment.
Not sure why it's marked as such, but it's not optional
|
|
||
| @Column({ nullable: true }) | ||
| @JsonRemoveEmpty | ||
| mention_everyone?: boolean; |
There was a problem hiding this comment.
Not sure why it's marked as such, but it's not optional
| nonce?: string; | ||
|
|
||
| @Column({ nullable: true, type: Date }) | ||
| @JsonRemoveEmpty |
There was a problem hiding this comment.
this is not a part of the message, and you can see the type includes null
|
|
||
| @Column({ nullable: true }) | ||
| @JsonRemoveEmpty | ||
| username?: string; |
There was a problem hiding this comment.
This does not matter as it's just being used then defaulted if nullish
| username?: string; | ||
|
|
||
| @Column({ nullable: true }) | ||
| @JsonRemoveEmpty |
There was a problem hiding this comment.
This does not matter as it's just being used then defaulted if nullish
|
Fixed in: #1594 |
|
Currently running this on our production instance with out any issues am curious what @s074, @CyberL1 or @TheArcaneBrony think of these changes? Can confirm, it does fix the issues with discord.py, and doesn't appear to break anything as far as i can tell |
|
Glad this solved the issue with Discord.py I think we should just create a separate DTO for message. Using our Message entity class as dto is both, polluting the entity class with serialization code, and possibly causing type problems since our message dto will have to include additional properties that need to be computed at send time. We are already encountering the second problem with MessageUpdateEvent which uses Message entity class as its type for We can create a separate type for a Message DTO, and also include a helper function for mapping a message entity object to dto |
I think this is the way to go. It feels like we've out grown it living in the Entity and its being pulled in too many different directions. |
|
if you look my recent changes, you mightve seen me trying to get rid of entities being used as DTOs as a whole |
|
Its also why this is still a draft, some changes have been imported into other PRs but there are lots of changes right now |
…ssage class. Hopefully shouldn't break anything.
d452006 to
f5c60a3
Compare
Added the JsonRemoteEmpty to more fields in the message class