Implement !tunnel command

[Matiiss]

Fixes #3512

[Matiiss]

I do have a couple of questions though, first, so currently I have made it so that only discord.TextChannel are supported, which means you can't use this command inside those voice channel text channels or threads, but I suppose it'd be fine to allow its usage there, too.

Another thing is that if I change the command method signature to use discord.abc.MessageableChannel, then suddenly !tunnel is being interpreted as a "tag" and it's trying to find a match for it among the other tags, instead of running this command. I'd like some advice on how to make it behave like it does with discord.TextChannel or like make the tag search skip it or something.

Then also, if I use the discord.TextChannel annotation (and presumably it would be similar if I fixed it so that discord.abc.MessageableChannel would work), if I provide a wrong ID (same length, just changed some digit), it doesn't appear to complain at any level, it seems it just sets that argument to None, which, I'm not sure if that's the behavior that I'd like. That is, you either provide a correct argument for the channel or it uses the default behavior without letting you know that you may have messed up.

Should there be checks for whether the given channel is a DM channel or some such? Currently it just seems to treat it as incorrect channel ID (probably because it's not a discord.TextChannel), so it just sets it to None, but if switching to discord.abc.MessageableChannel, that should probably be explicitly checked?

Similarly, what to do about ctx.guild, should that throw some more specific error or should it realistically never be None (which is my assumption, hence the AssertionError)? I used raise AssertionError` mostly for typing purposes here (in places where it also should never be realistically raised anyway).

Is there a simple way to do overloads for this command? So that with one argument it's !tunnel [destination_channel], but then with two it's !tunnel [source_channel] [destination_channel]?

Any shortcuts/convenience functions I should use for say permission checks from the existing code base?

Does this need unit or integration tests?

[jb3]

Nice initial implementation, a few feedback comments. Will address your other comments in a second.

[jb3]

This should be done in cog load listener, not in the cog init (see other cogs for prior art)

[jb3]

Use a guild-only check here instead, also maybe worth adding a cooldown decorator as we do to prevent abuse of this feature.

[jb3]

Feels cleaner to just do this:

Suggested change

	def get_least_active_channel_id(self, current_channel_id: int) -> int:
	"""Gets least active off-topic channel."""
	channel_id, _ = min(
	[
	(channel_id, timestamp)
	for channel_id, timestamp in self.channel_id_to_timestamp.items()
	if channel_id != current_channel_id
	],
	key=itemgetter(1),
	)
	return channel_id
	def get_least_active_channel_id(self, current_channel_id: int) -> int:
	"""Gets least active off-topic channel."""
	return min(
	(channel for channel in self.channel_id_to_timestamp if channel != current_channel_id),
	key=self.channel_id_to_timestamp.get
	)

[jb3]

I understand why you've got a few AssertionErrors throughout your implementation here, but this is not a pattern we have through the rest of this project and I'm not sure how I feel about implementing it just for this cog.

We have found that trying to do precise typing is almost impossible with how many updates the upstream discord.py types get as well as when new features are released to the Discord API that suddenly then start breaking assertions.

I'd rather see any genuine failures handled but otherwise do not think we should have these in the implementation.

[jb3]

so currently I have made it so that only discord.TextChannel are supported, which means you can't use this command inside those voice channel text channels or threads, but I suppose it'd be fine to allow its usage there, too.

Sounds fine — with the necessary cooldown added as mentioned so users cannot spam this in places we have less visibility.

Another thing is that if I change the command method signature to use discord.abc.MessageableChannel, then suddenly !tunnel is being interpreted as a "tag" and it's trying to find a match for it among the other tags, instead of running this command. I'd like some advice on how to make it behave like it does with discord.TextChannel or like make the tag search skip it or something.

This sounds weird, do you get the same if you try use GuildChannel instead?

That is, you either provide a correct argument for the channel or it uses the default behavior without letting you know that you may have messed up.

I wonder if using a https://discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.ext.commands.TextChannelConverter might help?

Should there be checks for whether the given channel is a DM channel or some such? Currently it just seems to treat it as incorrect channel ID (probably because it's not a discord.TextChannel), so it just sets it to None, but if switching to discord.abc.MessageableChannel, that should probably be explicitly checked?

You can use a guild-only decorator to enforce this.

Similarly, what to do about ctx.guild, should that throw some more specific error or should it realistically never be None (which is my assumption, hence the AssertionError)? I used raise AssertionError` mostly for typing purposes here (in places where it also should never be realistically raised anyway).

Same as above.

Is there a simple way to do overloads for this command? So that with one argument it's !tunnel [destination_channel], but then with two it's !tunnel [source_channel] [destination_channel]?

Not easily without doing most of the argument handling yourself.

I was going to suggest that for this initial implementation though we ditch the source channel support. Especially since the user who did the tunneling isn't mentioned in the tunneling messages it seems ripe for abuse (e.g. someone spamming in bot commands with another source & dest).

For now, let's keep things simple and just implement an optional destination channel.

Does this need unit or integration tests?

You can if you want to (and if you remove the source stuff it should be easier), we do have some utilities for mocking Discord components here and it's not too hard to add new bits.

[jb3]

Also, on the last point about abuse: can we include the user who tunneled on both the source and destination message (username is fine, doesn't need to be a mention), so that we can easily see any problems without having to look through message logs.