kordlib / kord

Idiomatic Kotlin Wrapper for The Discord API
MIT License
909 stars 80 forks source link

Added a function to load a message by a given message link. #951

Open Taubsie opened 3 months ago

Taubsie commented 3 months ago

I'm still unsure if I added the regex in the right place, if the implementation of the function is efficient enough and if this is needed at all. Could I get feedback on that?

DRSchlaubi commented 3 months ago

Maybe rebase this

Taubsie commented 3 months ago

@DRSchlaubi Could you tell me how?

DRSchlaubi commented 3 months ago

git rebate origin/main

Taubsie commented 3 months ago

git rebate origin/main

Current branch main is up to date.

What would be the issue?

lukellmann commented 3 months ago

I'm not sure if it's is a good idea to have a function like this as part of Kord. Just fact that the regex looks this complicated makes me skeptical. Also what would be a usecase for this function?

Taubsie commented 3 months ago

The usecase is being able to reference messages in commands in a clean way. Discord provides the function to easily copy a message's link, meaning you can simply copy-paste that. If you'd want to reference a message outside of the channel/server you're in, you'd be out of luck.

The regex looks complicated, I get that, but it's all documented - it just puts the parts of a message URL into regex groups

In previous frameworks I worked with that function was present, so I wanted to put it in here too, if that's not something you guys would deem fitting I can continue having it as an extension function, no problem

lukellmann commented 3 months ago

The usecase is being able to reference messages in commands in a clean way. Discord provides the function to easily copy a message's link, meaning you can simply copy-paste that. If you'd want to reference a message outside of the channel/server you're in, you'd be out of luck.

Message commands also exist but they probably don't work in all cases.

The regex looks complicated, I get that, but it's all documented - it just puts the parts of a message URL into regex groups

One more concern is that there might be more URL formats than anticipated - what if Discord decides to add another flavor in addition to ptb and canary, what if the URL contains a query or a fragment? Parts of this could be resolved to use an URI/URL class like io.ktor.http.Url instead of a regex.

In previous frameworks I worked with that function was present, so I wanted to put it in here too, if that's not something you guys would deem fitting I can continue having it as an extension function, no problem

What's your opinion @DRSchlaubi?

Taubsie commented 3 months ago

There might be new URL formats, that's correct. I'm unsure if that part of the API is documented, but I don't think so.

What I wanted to reach is creating a way to reverse basically a "mention" of a message - the counterpart of this function (the way the URL generated) can be found in KordEx: https://github.com/Kord-Extensions/kord-extensions/blob/e3da4b817e24f11d3130e442ef896801094a7157/kord-extensions/src/main/kotlin/com/kotlindiscord/kord/extensions/utils/_Message.kt#L242

Using both together would make sense imo, it's not urgent or needed at all like I mentioned

Replacing it with an URL probably works too, my question is how the parsing would look in that case? Wouldn't that end up with a regex as well?

lukellmann commented 3 months ago

Replacing it with an URL probably works too, my question is how the parsing would look in that case? Wouldn't that end up with a regex as well?

The parsing for URL classes is pretty general, so what you'd get is an object that is a valid URL and that you could then extract parts from. So the parsing would work with or without scheme, query and fragment but when using the URL object, you could simply ignore those and just use the host and path parts.

See e.g. https://github.com/ktorio/ktor/blob/main/ktor-http/common/src/io/ktor/http/URLParser.kt

DRSchlaubi commented 3 months ago

What's your opinion @DRSchlaubi?

I think that's more in @gdude2002's realm of things than ours