Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Taubsie
Copy link
Contributor

@Taubsie Taubsie commented Jun 8, 2024

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
Copy link
Member

Maybe rebase this

@Taubsie
Copy link
Contributor Author

Taubsie commented Jun 8, 2024

@DRSchlaubi Could you tell me how?

@DRSchlaubi
Copy link
Member

git rebate origin/main

@Taubsie
Copy link
Contributor Author

Taubsie commented Jun 8, 2024

git rebate origin/main

Current branch main is up to date.

What would be the issue?

@lukellmann
Copy link
Member

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
Copy link
Contributor Author

Taubsie commented Jun 8, 2024

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
Copy link
Member

lukellmann commented Jun 8, 2024

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
Copy link
Contributor Author

Taubsie commented Jun 8, 2024

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
Copy link
Member

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
Copy link
Member

What's your opinion @DRSchlaubi?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants