maxcutlyp / YoutubeBot

A self-hosted Discord bot for playing YouTube videos
51 stars 26 forks source link

Minor changes (.env & Intents support) #1

Closed KS-HTK closed 2 years ago

KS-HTK commented 2 years ago

I updated the Bot to support the required intents from discord.py

changed the token to reading from .env file. (create a file called .env with the line BOT_TOKEN=token. This is in my oppinion a better aproch than a token.txt file, it could hold more config information in future updates, and it does not require a newline at the end of the token. As not having this was causing errors.

Have not yet updated the readme.md to reflect the changes.

EDIT: Added the prefix to the .env file, as I had changed this in code and forgot to change it back in my original PR, a good example of why I think .env is better for config.

EDIT2: Added a default for the BOT_PREFIX, so if it is not in the .env it will use the original prefix. Added a check for a missing Token. Updated the README.md to explain how to add the token

maxcutlyp commented 2 years ago

Hi!

I like it. A .env file is definitely a better solution, and I like the prefix being customisable from it. A couple things I would change:

I hadn't heard about intents before this, but they look interesting. I do think it defeats the purpose if we just use all of them though. From the API reference, I think it should only need:

...but I haven't tested this. If you'd like, you could test this and see if there's anything missing from that list. I'm sure if there's anything it doesn't have access to, you'd get an error as soon as the bot tried to use it letting you know which intent is required (although again, I'm not 100% on that).

Thanks for your interest in this project! You've made me notice a few other things (like a big chunk of commented code and a typo in the readme) that I'm going to fix up now.

I look forward to hearing from you :)

KS-HTK commented 2 years ago

Thanks for the feedback, I will implement the suggestions in the next few days...

I just had a glance at the intent system. As this seems to be a rather new addition to Bot requirements, I went for the easiest implementation of it. It seems that any declared intents need to be met by the settings in the Discord Developer portal, but I got an error when the bot declared less intents than where permitted in the Developer portal. This requires some looking into.

I'll update this PR when I had the time to update my changes.

KS-HTK commented 2 years ago

My commit messages could have been a bit more clear. But I implemented all suggested changes.

Here a bit more description:

maxcutlyp commented 2 years ago

Looking good - a few things left, then I think we should be pretty much good to go! :)

If message_content is required, we'll need to make sure to add that to the readme. I'm thinking just an extra step at the end of the "Step 1: Creating an empty bot" section with some explanation as to why it's necessary, and moving the "make sure to save your changes" line to after it, like:

  1. From here you can give it a username. This is what it will show up as in your server.
  2. Scroll down to "Privileged Gateway Intents" and turn on the "Message Content Intent". This will allow the bot to read commands, like .play. Make sure to save your changes!

You can go ahead and remove token.txt from the gitignore as well - just make sure you also remove the file so you don't accidentally commit your token!

Additionally, I'm interested to know why you're catching SystemError around sys.exit(main()) - since the program would end anyway, wouldn't it be better to let the error kill it normally (print the stack trace, return an error code, etc)? Is the try-except necessary for another reason?

I'd also like to reword the line in the readme about the .env file - since the lines around it are talking about copying the token, it feels potentially confusing to talk about copying the file as well. Maybe "Rename the .env_example file to ..." instead of "Copy"?

I would also change "by the token" to "with the token" at the end of that line, but that's just me being pedantic.

Thanks again for taking the time for this, I really appreciate it!

KS-HTK commented 2 years ago

Removed token.txt from .gitignore

I left try except around the sys.exit() as it prevents dumping the stacktrace to server console. printing the stacktrace would be great in a debug/test mode, but having it in the server console could be annoying.

I left 'copy' for the .env file, but changed the order so it is more clear and you would not replace the copied token with the file. I don't think renaming the .env_example is what should be done, as it leaves the git in a changed state.

As per you're request i did change the wording to 'with the token'. These are things i would not notice as english is not my first language. So i am always glad to recieve this type of feedback.

EDIT: I updated the Title of the PR, just for future reference, to make it more clear what the PR was about.

maxcutlyp commented 2 years ago

I left try except around the sys.exit() as it prevents dumping the stacktrace to server console. printing the stacktrace would be great in a debug/test mode, but having it in the server console could be annoying.

Hmm, I see where you're coming from but I'm not sure I totally agree - having the stack trace in the server logs is useful for figuring out how to reproduce some corner case that occurred in production. Maybe there could be a PRINT_STACK_TRACE variable at the top of the script that controls whether the sys.exit call is wrapped in a try-except or not?

I left 'copy' for the .env file, but changed the order so it is more clear and you would not replace the copied token with the file. I don't think renaming the .env_example is what should be done, as it leaves the git in a changed state.

As per you're request i did change the wording to 'with the token'. These are things i would not notice as english is not my first language. So i am always glad to recieve this type of feedback.

I like it. Good call about leaving git in a changed state, I didn't think about that. I like the way it's worded now.

Just a few things around line 20 of the readme (about the message intent toggle):

Also, another minor thing, but in the error message about not having a token (on line 24 of youtubebot.py) only the first letter of the sentence should be capitalised (i.e. not "token" or "file").

Thanks again!

KS-HTK commented 2 years ago

I added the PRINT_STACK_TRACE option as an .env option, for easy disabling. It is set to default to true if not contained in .env file. Fixed the typos and wording in youtubebot.py and README.md

maxcutlyp commented 2 years ago

I like it - merged :)