[v3 cog manager, utils] handle missing cogs correctly, add some helpful algorithms #1989
Conversation
This file contains 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
calebj
force-pushed
the
v3/cog_manager_fix_async_utils
branch
from
075d7ed
to
e81d875
Compare
calebj
force-pushed
the
v3/cog_manager_fix_async_utils
branch
from
e81d875
to
100f0e5
Compare
calebj
force-pushed
the
v3/cog_manager_fix_async_utils
branch
from
100f0e5
to
45450c8
Compare
Tobotimus
added
QA: Unassigned
Type: Enhancement
For cog loading, only show "cog not found" if the module in question was the one that failed to import. ImportErrors within cogs will show an error as they should. - deduplicator, benchmarked to be the fastest - bounded gather and bounded async as_completed - tests for all additions
calebj
force-pushed
the
v3/cog_manager_fix_async_utils
branch
from
45450c8
to
3e47dcb
Compare
|
@Tobotimus rebased. |
Tobotimus
suggested changes
Aug 19, 2018
redbot/core/utils/__init__.py
Outdated
| loop = asyncio.get_event_loop() | ||
|
|
||
| if semaphore is None: | ||
| if type(limit) != int or limit <= 0: |
There was a problem hiding this comment.
Please use not isinstance instead of type() !=
redbot/core/utils/__init__.py
Outdated
| try: | ||
| done, pending = await asyncio.wait(pending, loop=loop, return_when=FIRST_COMPLETED) | ||
| except asyncio.CancelledError: | ||
| [] |
There was a problem hiding this comment.
Not sure what's going on with this [] statement, would prefer the use of pass or contextlib.supress
There was a problem hiding this comment.
I think I had logic here for cancelling pending tasks, but I found something better (see the push)
redbot/core/utils/__init__.py
Outdated
|
|
||
| async def sem_wrapper(sem, task): | ||
| async with sem: | ||
| return await task |
There was a problem hiding this comment.
Since this sem_wrapper is common to the one in bounded_gather_iter, it can be moved to the module-level and made private
redbot/core/utils/__init__.py
Outdated
| loop = asyncio.get_event_loop() | ||
|
|
||
| if semaphore is None: | ||
| if type(limit) != int or limit <= 0: |
redbot/core/core_commands.py
Outdated
| @@ -23,6 +23,7 @@ | |||
| from redbot.core import checks | |||
| from redbot.core import i18n | |||
| from redbot.core import commands | |||
| from redbot.core.cog_manager import NoSuchCog | |||
redbot/core/cog_manager.py
Outdated
| """ | ||
| resolved_paths = _deduplicate(await self.paths()) | ||
| resolved_paths = deduplicate_iterables(await self.paths()) |
There was a problem hiding this comment.
paths() already deduplicates the paths before it returns, so you can remove the call to deduplicate here
So I went source diving and realized as_completed works the way I want it to, and I don't need to reinvent the wheel for cancelling tasks that remain if the generator is `break`ed out of. So there's that.
mikeshardmind
approved these changes
Aug 20, 2018
Tobotimus
added
QA: Passed
Tobotimus
approved these changes
Aug 21, 2018
There was a problem hiding this comment.
Awesome. Thanks @calebj and thanks @mikeshardmind for reviewing
Jackenmen
added
Type: Bug
Jackenmen
added
Type: Bug
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.
Type
Description of the changes
For cog loading, only show "cog not found" if the module in question was the one that failed to import.
ImportErrorswithin cogs will now show an error as they should.Also:
asyncio.gatherand an asyncas_completed