Update information.py so it's a bit cleaner

[Denayder]

Hello! I was looking a bit around, and I stumbled over the information.py cog. I found a couple of things that I thought could be improved a slight bit, and went ahead and gave it a shot. If there are any further changes you would like me to make for this PR, please let me know and I will go ahead and do so 😁

Here’s a list of changes that I made:

• Improved imports a slight bit
• Instead of checking if every role name is @everyone or not, I simply use ctx.guild.roles[1:] instead, which skips the @everyone role. This was used in a couple of places, so it should generally be cleaner.
• When using the !role command, if the user parsed in a role that didn’t exist/couldn’t be converted, the bot would send a message and continue to the next role that the user entered. If the user entered 5 wrong roles, the bot would send 5 messages- and it could become spammy. Instead, all messages are parsed first, and if there were any roles that couldn’t be converted, the bot lets the user know in 1 message.
• I use the .__class__ attribute in a dict in order to get the text, voice and category channel count, instead of having a variable for each and if statements for each.
• Cleaned up the status checking a bit
• Reformatted strings so it’s consistent with other parts of the bot

Thank you for giving this PR a read! I look forward to hearing your feedback. 😄

[sco1]

Hello, thanks for the contribution!

A couple general requests before we open this up to a broader review:

• Please ensure that your fork is kept up to date with the latest changes to the upstream repository. This will help avoid merge conflicts (like we have here currently, since your fork is from November & 277 commits behind) and ensure that you have the most up-to-date functionality. GitHub has some guides to assist with this if you're not familiar with the process.
• Related to the above point, please make sure that you're working with a branch on your fork that isn't master. Once you start committing directly to your fork's master branch it makes it really difficult to synchronize yourself with any changes upstream.
• Keep in mind that the git history is for everyone's future benefit, including yours!
  • Try to keep commits as narrowly focused as possible. Commits that span broad areas of the code are difficult to review & keep isolated in the history.
  • Commit messages should be descriptive and provide insight into when and why things were done for future maintainers of the project. You can check out How to Write a Git Commit Message for a nice read on the topic.

And some more specific top-level reads:

• The import changes make a lot of modifications to the file with seemingly little gain. Without a better justification for the changes it would be better if they were left as-is.
• Same as above but for changing the block text from textwrap.dedent() to implicitly joined f-strings. Consistency with the rest of the bot probably isn't super critical here, and the joined f-string version arguably looks worse.
• The simplification to the channel & status counting is promising but can probably use some more work. Given that channels & statuses have related enumerations, collections.Counter may provide a more straightforward approach than the current refactor. While relying on __class__ seems to work fine, it seems more fragile than it needs to be.

[Denayder]

Hi - Thank you for the feedback, and apologies for the issues and commit-spam. I do admit I'm not the most experienced with git, so I apologize for the forking mistake and commit spam that has happened. Would you like this PR to be re-created so the commit history is not as messy?

As to your code requests:

• The import changes make a lot of modifications to the file with seemingly little gain. Without a better justification for the changes it would be better if they were left as-is.
  Is this regarding all of it, or just the constants? My justfifaction for some of it is:

• It seemed cleaner for me to import Union with the rest that were imported separately, rather than import all of typing, just to use Union.
• Same as above, but for the discord import; I believe only discord.Message was used, and I thought it would be cleaner and better in general to import Message separately with everything else if that was the case.

I can understand and do agree the change of constants being unnecessary, though. Would you like the constants to be reverted, or all of the import changes (e.g. the two ones I mentioned above)?

• Same as above but for changing the block text from textwrap.dedent() to implicitly joined f-strings. Consistency with the rest of the bot probably isn't super critical here, and the joined f-string version arguably looks worse.

This has been reverted, apologies.

The simplification to the channel & status counting is promising but can probably use some more work. Given that channels & statuses have related enumerations, collections.Counter may provide a more straightforward approach than the current refactor. While relying on class seems to work fine, it seems more fragile than it needs to be.

I'm not sure if I completely understand this. I've never worked with Counter before, but I gave the documentation a look and tried using it. I believe I've done what you've requested, but I'm not sure if you are saying that using __class__ is a bad idea and should be changed to something else. If so, is there anything specific that you propose?

Edit: On another note, I have zero clue why that check is failing. This works locally, and I have zero clue why member.status is returning a string.

[sco1]

No worries, it's all a learning experience. If you wouldn't mind, let's start from scratch & on a new branch so your master is clean and in sync with our repo and we don't have all the unnecessary commits. Then we can dive into everything else.

Regarding the failing test, it's an issue with how we're mocking a member list for the test:

bot/tests/bot/cogs/test_information.py

Lines 127 to 131 in ca0ee4a

	members=[
	*(helpers.MockMember(status='online') for _ in range(2)),
	*(helpers.MockMember(status='idle') for _ in range(1)),
	*(helpers.MockMember(status='dnd') for _ in range(4)),
	*(helpers.MockMember(status='offline') for _ in range(3)),

With the old logic in the information cog, using strings rather than discord.py's Status enum was sufficient, but since our logic is changing then the tests will need to be adjusted as well. It would be appropriate to do this as part of this PR.

[Denayder]

Awesome, I will create a new PR once it is ready. Thank you for the valuable feedback so far. :)