[WIP] fixes issue 112; add client var to use userland DiscordClient in library code

[nayr7]

This PR modifies startSession to assign a reference to a variable that points to a user populated DiscordClient so that we can use it in library code, this allows us:

• to introduce procs like defer and other useful sugar that doesn't need the discord.api prefix
• to (?) potentially refactor a bunch of procs that would not need the prefix either

New sugary procs are noticable by their short naming scheme.

What we need to complete this PR:

• [ ] tests that include the added procs
• [ ] documenting them
• [ ] decide if this is the design that we really want

[krisppurg]

If you're adding helper procs I think adding them to the dimscord/helpers.nim file would be ideal.

If you're adding the helper procs based of only just message and interaction (Message.react/edit/reply), then I think the PR is more suited for dimscmd.

If somebody wants to say editGuild, banUser, getGuildAuditLogs etc they still have to use discord.api prefix.

[nayr7]

If you're adding the helper procs based of only just message and interaction (Message.react/edit/reply), then I think the PR is more suited for dimscmd.

They are just the usecases that came to my mind first, with this PR we can open the way for more helper procs but if you want we can get all of them done in this PR instead of doing it little by little. Or I'll just make a PR for dimscmd instead if you really don't want to.

If somebody wants to say editGuild, banUser, getGuildAuditLogs etc they still have to use discord.api prefix.

Well, making a sugar for them is doable and it's just a matter of making the procs. That's why it's "WIP", I know it's merely "saving a couple of typing" but I think it's also about having more consistency. Want to "edit" a Guild/User/Role/Etc ??? Just use edit on them, it's like a better ctx if you're coming from Python and all this overloading makes dimscord (and by extension bot development in Nim) a lot more approachable/comprehensive/familiar for those coming from other languages who have similar idioms.

If you're adding helper procs I think adding them to the dimscord/helpers.nim file would be ideal.

Sure thing :)

[nayr7]

The way my lib worked was by wrapping types in a generic type that looked like this (I can't remember the exact >implementation)

type
  EasyDiscord*[T] = object
    data: T
    api: RestApi 

This allowed me to add a reference to the API without needing to modify dimscord

I was doing something similar too but by using inheritance, because I needed a base type for implementing a generic waits table in order to wait for an event. It seems like a better design than globals indeed and we would "kill two birds with one stone" if we do it this way.

[ire4ever1190]

After seeing #106 I think inheritance is the way to go :+1:

[nayr7]

Pushed some more procs. However you will notice a lot of them are commented, this is because:

• some fields are not accessible like guild_id, which could be added to relevant objects if we extend todo 1 of https://github.com/krisppurg/dimscord/issues/108. Other discord libraries also do this btw.
• the object does not inherit from DiscordEvent.

... which brings me to what I mentioned a couple days ago on discord, I believe that having to turn a ton of objects into ref object because i need them to inherit the client is not worth it and doesn't make sense anyways. I used to say inheritance would be a "great" solution because we would also use it to implement https://github.com/krisppurg/dimscord/discussions/106 but I am starting to explore more options that doesn't involve it.

In Nim it's recommended to have as little ref object as possible. I believe there is a worthwhile undertaking and legitimate interest to remove some refs and use var parameters if we need mutation instead... But that's not the point. What I mean is that we got what I believe to be a better solution in the form of public getter function with {.global.} local var, aka the second iteration of this PR albeit tweaked slightly. It should be pretty safe as long as we don't use threads (and even then the proc would just copy instead of passing the ref so it should still be safe but with some latency due to the copy) and yes it should be updated with the latest data from discord to keep ratelimits at bay, we would just need to test if the behavior is one that is intended because {.global.} seems to have some quirks.

[nayr7]

Also please note that if we go with inheritance, this PR becomes a breaking change

[nayr7]

Ready for review and validation, please refer to my past comments if you wonder why I removed inheritance and opted for another approach. also @ire4ever1190 why the confused react ?

[ire4ever1190]

Confused why you moved from inheritance. I'm reading the messages, but I still think a global variable is a bad idea. I also find the getClient which also acts as a setter to be weird. I assume it was so the client couldn't be reassigned but that is still possible like so

let foo = newDiscordClient("Some token")
getClient()[] = foo[]

[nayr7]

Confused why you moved from inheritance.

Well that is because it's harder to implement and has a big shotgun which is due to a limitation of this approach:

1. First of all, the shotgun in question is that for objects that are obtained by using the API and assuming there is no global var involved, their client field will be nil (or none DiscordClient) because all the getters that rely on the API do not have the data, unless you do discord.stuff(myobj, args) instead of myobj.stuff(args) and assign discord to the client field before you return the object but then what's the point ? You might as well just do discord.getStuffFromApi(myobj.id, args) which is exactly the sort of syntax this PR aims to make nicer.
2. For objects that come from event handlers and are cached, the client field would be present if we modify every event handling procs. But not every object come from event dispatchers, for example Application ... So this mean the only plan is to not do anything about it instead, no helping procs for Application and more.
3. You already know but having to use inheritance means turning some objects into ref object if we plan to implement https://github.com/krisppurg/dimscord/issues/105. I feel like it's too early to decide if we're really going to use this approach and instead I suggest to take a step back instead and think through all our options which is also part of why I changed my mind because I though I was being too eager.

Now, because of point no.1, it could be that we need Option[DiscordClient] instead, but this doesn't change much unfortunately. The cost of using inheritance for everything must not be neglected too.

[nayr7]

but I still think a global variable is a bad idea

Maybe it is but I am trying to think of every edge case and gotchas, it should be fine if we cannot write back to the ref. Remember that users set a discord global var on their code and use it all over the place because the library told them to do so. The goal of this PR should be to make it as safe or even safer as using discord directly.

let foo = newDiscordClient("Some token")
getClient()[] = foo[]

... which is why I am going back to the drawing board 😅

[nayr7]

btw, I would be fine with inheritance, but I really don't know what to do about those issues, would appreciate some guidance here

[ire4ever1190]

In regards to the limitations

1. If obtained through the API then the client field shouldn't be null. Would require updating a lot of the API procs but mostly a matter of making sure to assign the client field and also taking the client as a proc instead

   proc getChannelPins*(discord: DiscordClient,
       channel_id: string): Future[seq[Message]] {.async.} =
   ## Get channel pins.
   result = (await discord.api.request(
       "GET",
       endpointChannelPins(channel_id)
   )).elems.mapIt() do:
       m = newMessage(it)
       m.client = discord
       m

   This is assuming they need the actual DiscordClient and not just the RestAPI. @krisppurg are there any procs that would require the full client or is passing around the RestApi object enough? If just passing RestApi around is enough then ignore the next paragraph (besides maybe the with proc, that could still be handy)

Since that is a massive breaking change we could also just add a with proc like below and then save changing parameters for a 2.0 breaking update

# We make it generic so the original type isn't erased
proc with[T: DiscordClient](e: T, client: DiscordClient): T {.inline.} =
  e.client = client
  e

2. Things like commands can still be got via the api with things like getApplicationCommands so would be good to have helpers for those. But since they are also constructed manually, it would just require the user to also pass the client when constructing or use something like the with proc I mentioned

3. Smart move to take a step back and plan. Currently I think the design is fine but guess we won't know until we try. I'll see if I can get an MVP done today to give us a better idea on how it will work

[ire4ever1190]

(If Krisp says that passing around RestApi is enough, then replace all usages of client with api in my previous message)

[ire4ever1190]

nvm, current MVP of awaiting would require client anyways

[krisppurg]

Re:

@krisppurg are there any procs that would require the full client or is passing around the RestApi object enough? If just passing RestApi around is enough then ignore the next paragraph (besides maybe the with proc, that could still be handy)

Im pretty sure there are no procs that require the full client, RestApi is enough as the necessary information it has the api version and the discord bot token.

[nayr7]

further developement of the inheritance approach happens here: https://github.com/krisppurg/dimscord/pull/113