TextChannel.clone() does not copy all attributes

[onerandomusername]

Summary

Copying a text channel (and probably others) is not an exact copy of a channel

Reproduction Steps

Create or modify a channel

Clone it using the clone method.

Minimal Reproducible Code

# first modify the channel to have most attributes filled
await channel.edit(topic='h', slowmode_delay=4, nsfw=True, default_auto_archive_duration=disnake.ThreadArchiveDuration.three_days)
# then call clone and look at the new channel
newchan = await channel.clone()

await channel.send(newchan.default_auto_archive_duration == channel.default_auto_archive_duration)

Expected Results

All attributes are copied.

Actual Results

default_auto_archive_duration is not copied.

Intents

--

System Information

- Python v3.8.12-final
- disnake v2.6.0-alpha
    - disnake pkg_resources: v2.6.0a4296+g0fcc655d
- aiohttp v3.8.1
- system info: Linux 5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022

Checklist

• [X] I have searched the open issues for duplicates.
• [X] I have shown the entire traceback, if possible.
• [X] I have removed my token from display, if visible.

Additional Context

Found this while testing #418, tested it on master and same issue. The issue is basically the underlying _clone_impl which should probably be refactored a bit to not require presetting specific attributes on a per channel basis. https://github.com/DisnakeDev/disnake/blob/9e716dd2ffc7641142fb59edb33d7ec295eb9b43/disnake/channel.py#L414-L418

[ChristopherJHart]

I'd be willing to take a crack at this! I'd be curious to hear any ideas for avoiding presetting specific attributes on a per-class basis.

From what I can tell, attributes that control channel configuration are present in valid_keys in disnake.http.create_channel():

https://github.com/DisnakeDev/disnake/blob/9d53db00a50ecdac1aa574f3d700878e50297bb7/disnake/http.py#L1038-L1052

What if we did the following:

1. Refactor valid_keys into a TypedDict in disnake.types.channel.py, maybe named ChannelConfiguration.
2. Refactor disnake.http.create_channel() to pull data from the options argument and insert it into payload using the keys of ChannelConfiguration. At the end of the day, disnake.http.create_channel() behavior will be unchanged.
3. Reference the keys of ChannelConfiguration in GuildChannel._clone_impl(). Cleverly use getattr against self to pull relevant attributes of GuildChannel subclasses and slide them into base_attrs when they're not None.
4. Remove base_attrs from the arguments of GuildChannel._clone_impl() and refactor invocations of GuildChannel._clone_impl() accordingly.

Thoughts?

[onerandomusername]

Hello! Thank you for wanting to solve this issue! We very recently merged #418 which internally defines a list of channel attributes. On this regard, it may be more efficient to define a ClassVar tuple of tuples of api attributes to channel attributes to map when cloning and editing.

We could then do something like the following:

{
  "nsfw": "nsfw",
  "topic": "topic", 
  "rate_limit_per_user": "slowmode_delay"
}

From this list, it is possible that GuildChannel.edit could additionally use this, but not sure. @shiftinv, would you please verify?

Additionally, the suggestion about using disnake.types.channel.py just isn't doable: disnake.types is not importable at runtime, nor is it worth the effort to make it so.

The rest of that sounds good to me, though we'll need the input from other maintainers.

[ChristopherJHart]

I want to make sure I understand the proposed implementation before I move forward on doing the thing 😅 So you're saying under disnake.abc.GuildChannel, we create a class variable that maps API attributes to channel attributes. Then, disnake.abc.GuildChannel._edit() constructs the options dictionary based on this class variable before passing it to disnake.http.edit_channel(). We would then repeat this same design pattern for disnake.abc.GuildChannel._clone_impl().

This makes sense, but the only concern I would have is how disnake.http.edit_channel() still uses valid_keys to extract data from the options parameter. Therefore, if we needed to add a new option, we'd need to update valid_keys and the class variable that maps API attributes to channel attributes. Unless I'm missing something, we can't import disnake.abc.GuildChannel into disnake.http because then we'll have a circular import.

My thinking is that ideally, the API/channel attribute mapping is abstracted away from disnake.abc.GuildChannel entirely as a model that we can import into disnake.abc.GuildChannel and disnake.http (maybe we throw it into disnake.utils?). That way, if we need to add a new option, we just add it to the abstracted model, and the model-parsing code automagically handles it. Assuming this works, we could follow this same design pattern with methods that create channels as well.

[shiftinv]

Deduplicating code is definitely a great idea, but we might lose out on type-safety depending on how it's implemented (though the current code paths are already sprinkled with typing.Any so meh). Regardless, valid_keys in http.edit_channel could additionally just be removed altogether, it doesn't really serve a purpose.

Since the channel create endpoint doesn't necessarily support all fields (at least currently, forum channels can't be cloned easily due to tags), there's no guarantee that one request would suffice in all cases if we were to clone all fields instead of replicating client behavior. A separate method for a "full" clone that isn't atomic might be a good solution though, imo. (also briefly discussed on Discord here)

[onerandomusername]

What was discussed on Discord is essentially we would implement two clone methods.

One would be to copy the client when the client clones a channel, and the other would be to clone a full (or nearly, as some things cannot be made at channel create) channel copy.

One way would be two methods, something like clone and full_clone. Please note names are not final. clone would keep its existing behavior, while full_clone would clone all attributes, like default_auto_archive_duration and more.

The alternative is to implement a parameter to clone which would do a full clone with all attributes. For example, clone(full=False) would follow the client implementation, but clone(full=True) would clone all clonable attributes.

[onerandomusername]

A separate idea I was thinking of for a new feature would be the ability to change one or more attributes when creating the clone. The method would take nearly the same attributes as channel.edit() but if not provided they would default to the current channel information.

A channel that has a topic of Get help and multiple other attributes could be cloned but with a different topic by using clone(full=True, topic='docs') or full_clone(topic='docs')

[ChristopherJHart]

Going to try to take care of this tomorrow! For this specific issue, I'll keep the scope simple and just fix the channel attributes (e.g. default_auto_archive_duration) used by clone() such that they match the client. I'll then create another issue for the full_clone() and/or full=True feature and work on that, assuming y'all are okay with that!

[onerandomusername]

Yes, that sounds good