Incorrect handling of tags by the `!source` command

[mbaruh]

When trying to use the source command on a tag, it errors out and the exception is not caught:

A simple solution would be to just catch the exception, but since it's a very identifiable scenario (being an instance of TagIdentifier), we can instead point to the appropriate MD file in the repo.

EDIT: This actually seems to have worked before #1663

[onerandomusername]

I can implement this tonight, if no one else wants to do it before then

[Numerlor]

Looks like this is caused by how we're loading extensions, if the source cog is loaded before the tags cog, then the TagIdentifier class used by source is different to the one in the tags cog because the tag module is reloaded when the extension is loaded.

Although when it does work it uses the wrong path so that should be fixed in the source cog at L68

[onerandomusername]

Looks like this is caused by how we're loading extensions, if the source cog is loaded before the tags cog, then the TagIdentifier class used by source is different to the one in the tags cog because the tag module is reloaded when the extension is loaded.

Although when it does work it uses the wrong path so that should be fixed in the source cog at L68

Yeah, I moved TagIdentifier to a separate file, and that it solves the problem.

[Numerlor]

Looks like this is caused by how we're loading extensions, if the source cog is loaded before the tags cog, then the TagIdentifier class used by source is different to the one in the tags cog because the tag module is reloaded when the extension is loaded. Although when it does work it uses the wrong path so that should be fixed in the source cog at L68

Yeah, I moved TagIdentifier to a separate file, and that it solves the problem.

While that does solve the problem, I feel like it's more of a bandaid solution. A similar error is likely to occur for other extensions if they end up depending on each other so I think something more robust like making loading extensions respect already imported modules instead of overwiting them would be better here. Or at least making the extensions order deterministic so the errors caused by it are not intermittent.

[onerandomusername]

While that does solve the problem, I feel like it's more of a bandaid solution. A similar error is likely to occur for other extensions if they end up depending on each other so I think something more robust like making loading extensions respect already imported modules instead of overwiting them would be better here. Or at least making the extensions order deterministic so the errors caused by it are not intermittent.

We shouldn't be importing from extensions since reload and loading will now have undefined behavior. I don't think we import from extensions that much...

oh no

In order to make loading extension respect already imported modules, we would need to get deep into the implementation of discord.ext.commands.Bot.load_extension, and importlib

Here's the core magic of extension loading. https://github.com/Rapptz/discord.py/blob/45d498c1b76deaf3b394d17ccf56112fa691d160/discord/ext/commands/bot.py#L655-L679

Any change we make to this would make reload_extension have even more undefined behavior.

We may also need to write code to figure out and track imports and calculate a dependency tree to order the extensions.

[onerandomusername]

UPDATE: after futher invesigation, most of the above imports are being used for type hinting, and the cogs do use get_cog

There's one case where I found one cog being imported from another module, which is below. Simple matter of updating to get_cog https://github.com/python-discord/bot/blob/c4837978399ce42b7073e17bae7e30b7a43d088d/bot/exts/moderation/watchchannels/_watchchannel.py#L16

Most of the 47 above imports could be done up like the below: https://github.com/python-discord/bot/blob/db85e56baf7edbd204fae42572d01923ec398840/bot/exts/recruitment/talentpool/_review.py#L25-L26

[Akarys42]

I think get_cog would be the right way to go for any inter-cog dependency. In this case, though, it is a bit different since we have a utility class in the same file as the cog. I think moving to another file sounds pretty good. I would say we should avoid making any changes to the extension loading mechanism.

[Ibrahim2750mi]

This can be closed now as the source commands works for tags.

[wookie184]

The issue is still there, although only if the cogs are loaded in a certain order. Running !ext reload tags should allow you to reproduce the issue.

[hedyhli]

While the solution presented in the PR above works (by not checking for isinstance of TagIdentifier entirely), I don't think it's very robust in the long run.

It looks like SourceConverter is always able to identify tags to be tags, so why not we use a util function that will return the source-converted object, as well as a enum value that represents the type of object the source-object is?

This skips checking for isinstance entirely, for all SourceTypes, and retains the existing logic of if-checks without resorting to TagIdentifier as a fallback.

(Draft implementation: http://0x0.st/Xs2e.patch - not very polished, just a proof-of-concept!)

What do we think of this solution?

[wookie184]

@hedyhli I agree that your proposal sounds more robust. Lets do it. Were you planning to do the PR for it?

[hedyhli]

Sure, I'll see to it once I'm free within the next week or two.