Put cogs in redbot.ext_cogs package + refactor CogManager#6693
Open
Jackenmen wants to merge 4 commits intoCog-Creators:V3/developfrom
Open
Put cogs in redbot.ext_cogs package + refactor CogManager#6693Jackenmen wants to merge 4 commits intoCog-Creators:V3/developfrom
Jackenmen wants to merge 4 commits intoCog-Creators:V3/developfrom
Conversation
Co-authored-by: Toby Harradine <me@tobyharradine.id.au>
The deep reload proposed by Toby runs into some problems with the package ending up with a mix of objects (classes, functions, etc.) from both old and new module.
Member
Author
|
For future reference, here's a script for finding cogs in the Index that are affected by the changes in this PR: check_6693.pyimport re
import textwrap
from pathlib import Path
total_repo_count = 0
total_cog_count = 0
repo_count = 0
cog_count = 0
for repo_path in Path("cache").iterdir():
if not repo_path.is_dir():
continue
total_repo_count += 1
repo_name, *_ = repo_path.name.rpartition("_")
repo_matches = {}
for cog_path in repo_path.iterdir():
total_cog_count += 1
if not cog_path.is_dir():
continue
cog_name = cog_path.name
escaped_cog_name = re.escape(cog_name)
pattern = re.compile(
fr"from +{escaped_cog_name}(\.[a-zA-Z0-9_]+)? +import"
fr"|import +{escaped_cog_name}(\.[a-zA-Z0-9_]+)?($| |#)"
)
cog_matches = {}
for file in cog_path.rglob("*.py"):
with file.open(encoding="utf-8") as fp:
for match in pattern.finditer(fp.read()):
file_matches = cog_matches.setdefault(file.relative_to(cog_path), [])
file_matches.append(match)
if cog_matches:
cog_count += 1
repo_matches[cog_name] = cog_matches
if repo_matches:
repo_count += 1
print("Found matches in", repo_name)
for cog_name, cog_matches in repo_matches.items():
print(f' - Cog "{cog_name}":')
for file, file_matches in cog_matches.items():
print(f' - File "{file}":')
for match in file_matches:
print(f" {match.group()}")
print()
print(
f"{repo_count} repos out of {total_repo_count}"
f" ({repo_count / total_repo_count * 100:.2f}%) affected"
)
print(
f"{cog_count} cogs out of {total_cog_count}"
f" ({cog_count / total_cog_count * 100:.2f}%) affected"
)The results I got are: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the changes
This is pretty much a rehash of #2446 updated to work with the current codebase. Here are the relevant parts of the description from that PR:
The only major deviation from the code of the original PR is the reload logic - the logic proposed by Toby is meant to deep reload the package, but a simple loop over
sys.modulesis not enough to achieve this, and what ends up happening instead is the package ends up with a mix of objects (classes, functions, etc.) from both the old and new module. This can cause some actual issues, as evidenced by, e.g. trying to load the Audio cog.Instead, the reload logic left is kept pretty much as is, except that it uses
importlib.reload()rather than the internalimportlib._bootstrap._exec().importlib.reload()does useimportlib._bootstrap._exec()under the hood, so the observed behaviour should be identical.Fixes #5956
Have the changes in this PR been tested?
Yes