krisppurg / dimscord

A Discord Bot & REST Library for Nim.
https://krisppurg.github.io/dimscord/
MIT License
222 stars 20 forks source link

fix createWebhook, executeWebhook(wait=false) and with some code fixes #55

Closed pchicken closed 2 years ago

pchicken commented 3 years ago

fixes to two bugs I noticed when using createWebhook and executeWebhook(wait=false)

krisppurg commented 3 years ago

When does message data get nil? @pchicken

krisppurg commented 3 years ago

You could append discard await when wait=false, so discard await discord.api.executeWebhook(... , wait=false).

Unless if that returns nil.

pchicken commented 3 years ago

I used discard await with wait=false originally as per instructions. When wait=false, the request returns nil, which is passed to newMessage to form executeWebhook's result (which leads to an AssertionError from attempting to access a nonexistent key, since data is nil).

This all happens inside executeWebhook, so appending discard await before executeWebhook doesn't do anything for this error.

krisppurg commented 3 years ago

Ah, I see. I wouldn't return Message as Message() when nil.

The best option I could think of is changing return the executeWebhook proc with Future[Option[Message]], same goes any proc that has wait in it, since they either produce the same issue; Example code for my suggestion

let fut = executeWebhook(... , wait=false)
if fut.isNone:
  echo "There is nothing"

let fut2 = executeWebhook(... , wait=true)
if fut2.isSome:
  echo "Here is the msg: " & $(fut2.get[])
krisppurg commented 3 years ago

@pchicken

pchicken commented 3 years ago

Sorry

krisppurg commented 3 years ago

I'm not exactly asking you to close your PR, you can still open.

spx01 commented 2 years ago

May I ask why this hasn't been merged yet? The createWebhook fix is quite important.

krisppurg commented 2 years ago

There are large merge conflicts, and I didnt want to delay dimscord v1.3.0 because of the time it takes. I postponed it, but I'll just close the PR and just add commits based on this PR.

krisppurg commented 2 years ago

Due to merge conflicts and issues, I thought it would be a lot more easier if I could just simply make a commit resolving the issue https://github.com/krisppurg/dimscord/commit/8b4dc08f821c6b0f12aab95483765281bc78c359