richfromm / slack2discord

A Discord client that imports Slack-exported JSON chat history to Discord channel(s).
GNU General Public License v3.0
8 stars 3 forks source link

Parsing error of channel names with multiple dashes #24

Closed shmulvad closed 1 year ago

shmulvad commented 1 year ago

One of my Slack channels was named something similar to string1---string2. This got parsed as string1-string2 when creating the channel, but as string1---string2 when trying to post messages to the channel (which then failed because a channel with that name hadn't yet been created). This should be fixed so it is consistent.

richfromm commented 1 year ago

Apologies for taking so long to respond to this. I didn't see any of these open issues until yesterday. See https://github.com/richfromm/slack2discord/issues/22#issuecomment-1420214552

I am able to locally reproduce the issue, see attached log.

slack2discord.log

richfromm commented 1 year ago

I can't find anything documenting it (certainly not in the create_text_channel() docs), but empirically it looks like Discord doesn't allow channels to have multiple dashes.

This is even outside of the API, doing it manually within the GUI, I can't type in a second dash in the popup for entering the name for a new channel. It just doesn't recognize that I've typed that character.

But the API behavior is a little weird. Rather than raise an error if you pass an invalid name, it instead just converts multiple dashes into a single dash, creates the new channel with that modified name, then returns that name to you as the new channel. And that causes an assertion of mine to fail, since it doesn't match what's expected: https://github.com/richfromm/slack2discord/blob/2617f7e5b5bb85d14a24a4b4e93de4b30f5df73a/slack2discord/client.py#L159 . That seems like a really bad idea to me.

I suppose we could just go along with this, and automatically do the same, but I don't like the idea of doing that. What if you have both foo-bar and foo--bar, what should we do then? I think I'd rather just detect this situation and alert the user if there are channels with illegal names, and fail early (with a suitable error message), and let the user resolve this. Esp. since there is already a mechanism for having different channel names in Discord compared to Slack, via the --channel-file option.

It also appears that empirically Discord doesn't allow spaces in channels, so there's the question as to whether we should include that in the channel name validation or not. Their treatment of spaces is a little different, if you type one in the GUI it actually translates that to a dash instead. (I haven't verified if the same thing happens with the API, but I'd guess so.) And then a second space just doesn't work, just like a second dash doesn't work.

However, it appears that Slack also doesn't allow for spaces, so maybe this isn't worth worrying about in practice? I'm undecided, but I might just toss it in anyway, since if I'm going to add some channel name validation and an error msg to inform the user about how to deal with it, it's pretty trivial to add one more criteria to the validation check.

richfromm commented 1 year ago

Allegedly there is no limit on channel name length in Discord (unlike Slack), see https://discord.com/moderation/208-channel-categories-and-names#title-2

So I think I will limit validation to checking for repeated dashes, as well as any spaces (or perhaps any whitespace).

UPDATE: In the Discord GUI, trying every non-alphanumeric character on my (US) keyboard, the only non-alphanumeric characters accepted are dash (-) and underscore (_). This is consistent with Slack, but I can't find that documented anywhere for Discord. I will do some experimenting and see if the API is consistent with the GUI or not, regardless of whatever the documentation does not say.

richfromm commented 1 year ago

I did some tests regarding channel creation. It would be nice if this were just documented. Here's what I found regarding channel names:

An additional observation is that if you try to create a channel with a name that already exists, that succeeds, a new channel is created with a dupe name. I don't want to put too much energy into dealing with this, b/c IMHO it's a really stupid thing to do. Regardless, right now in DiscordClient.get_channel() I'm assuming that this can't happen, and if it does I log an error and raise RuntimeError. Perhaps a better strategy would be to just pick the first one found and log a warning. Esp. if we end up making the default behavior to prompt the user to review the channel situation before proceeding, see https://github.com/richfromm/slack2discord/issues/23#issuecomment-1423081840

Anyway, given all of the various undocumented restrictions, I think my validation criteria will be:

UPDATE: I hadn't explicitly tested for this before, but double underscore is fine, as is underscore followed by dash, and dash followed by underscore. It's only multiple dashes in a row that are a problem (even dash underscore dash is fine).

richfromm commented 1 year ago

See my current plan here

richfromm commented 1 year ago

Somewhat change of plan. I still like the idea of querying Discord state early, and (by default) prompting the user for confirmation regarding the category/channel situation, as outlined above. And I will try to get to it in the not too distant future.

But, for reasons discussed earlier, it's non-trivial to implement, given the async nature of the Discord API.

So for the short term resolution of this issue, I think it's fine to validate all of the supplied Discord channel names (and fail fast if any do not), early in the process, without querying Discord state to see whether or not the channels exist.

Similar short term change of plan at https://github.com/richfromm/slack2discord/issues/23#issuecomment-1430808368