YouTube Downloader with PyTube

Question

I've made a little app, YouTube Downloader, which does what the name says.
I have three files as I am trying to get good enough in working with multiple files in one app. So even if it is really small I will try to separate it.

I have some questions:

• Is my style acceptable?
• Is the code clean enough(I know there is not much code in there ;))?
• Is readability ok?

In short; if you gave me a task to create this and I give you this, would you be happy?

MAIN.PY

import sys, os
sys.path.append(f'{os.path.dirname(__file__)}\\scripts')
from scripts.app import App
from scripts.downloader import Downloader

def main() -> None:
    _eng = Downloader()
    App(engine=_eng).mainloop()

if __name__ == '__main__':
    main()

APP.PY

import tkinter as tk
from tkinter import BooleanVar, PhotoImage, StringVar, ttk
from functools import partial
from typing import Any

class App(tk.Tk):
    def __init__(self, engine: Any) -> None:
        super().__init__()
        self._eng = engine

        self.title('Youtube Downloader')
        self.iconphoto(True, PhotoImage(file='icon.png'))
        self.geometry('700x300+400+300')
        self.resizable(False, False)
        self.configure(background='#333333')
        self.style = ttk.Style()
        self.style.configure('TCheckbutton', background='#333333', foreground='white')
        self.style.configure('TButton', font=('Tahoma', 10, 'bold'), background='#333333')

        self.link_var = StringVar()
        self.mp3_var = BooleanVar()
        self.file_name = StringVar()
        self.file_path = StringVar()
        # partial used for labels instead of style as it is minimazing code to variable name and text only
        label = partial(ttk.Label, self, font=('Tahoma', 10, 'bold'), background='#333333', foreground='white')

        link_label = label(text='Enter URL:')
        link_label.place(x=10, y=10)
        link_entry = ttk.Entry(self, textvariable=self.link_var)
        link_entry.place(x=10, y=40, width=680, height=30)
        label_mp3 = label(text='Download as MP3:')
        label_mp3.place(x=10, y=75)
        mp3_check = ttk.Checkbutton(self, variable=self.mp3_var, text='MP3', onvalue=True, offvalue=False, style='TCheckbutton')
        mp3_check.place(x=150, y=75)
        download_button = ttk.Button(self, text='DOWNLOAD', style='TButton', command=lambda: [self._eng.download(self.mp3_var.get(), self.link_var.get()),
                                                                                              self.file_name.set(self._eng.download(self.mp3_var.get(), self.link_var.get())[0]),
                                                                                              self.file_path.set((self._eng.download(self.mp3_var.get(), self.link_var.get())[1]))])
        download_button.place(x=250, y=110, width=200, height=40)
        file_info_label = label(text='File:')
        file_info_label.place(x=10, y=200, height=30)
        file_info_label_var = label(textvariable=self.file_name)
        file_info_label_var.place(x=40, y=200, height=30)
        path_info_label = label(text='Downloaded to:')
        path_info_label.place(x=10, y=240, height=30)
        path_info_label_var = label(textvariable=self.file_path)
        path_info_label_var.place(x=120, y=240, height=30)
        
        self.file_path.trace_add('write', self.empty_entry_field)
    def empty_entry_field(self, x: Any, y: Any, z: Any) -> None:
        self.link_var.set('')

DOWNLOADER.PY

from pytube import YouTube
from pathlib import Path

class Downloader:
    def __init__(self) -> None:
        super().__init__()

    def download(self, MP3BoolVal: bool, url: str) -> list[str]:
        downloads_path = str(Path.home() / 'Downloads')
        video = YouTube(url)
        name = video.title
        # / and \ need to be removed from the name. otherwise tkinter calls OSError as it's taking it as part of path
        name = name.replace('\\', '').replace('/', '')
        if MP3BoolVal:
            new_name = f'{name}.mp3'
            print(video.streams.get_audio_only())
            video.streams.get_audio_only().download(filename=new_name, output_path=downloads_path)
        else:
            new_name = f'{name}.mp4'
            print(video.streams.get_highest_resolution())
            video.streams.get_highest_resolution().download(filename=new_name, output_path=downloads_path)

        return [new_name, downloads_path]

Answer 1

"MP3 or not" isn't as informative as "MP3 or MP4", so you should convert your checkbox to a couple of radio buttons.

This is spooky:

sys.path.append(f'{os.path.dirname(__file__)}\\scripts')

and probably means that your directory structure is misdesigned. If you want to put other scripts in a subdirectory, it should be as simple as making it a normal Python module with an __init__.py and importing it with a fully-qualified name.

Don't hint your engine as an Any: it's a Downloader.

x: Any, y: Any, z: Any is not the right signature for a trace; it should be name: str, index: str, mode: str. If you truly don't care about the signature at all, you could just leave it as *args.

list[str] should actually be Tuple[str, str], and your return should just be made the standard

return new_name, downloads_path

Answer 2

Do not repeat potentially expensive operations

This snippet is used as a command to execute when clicking on Download:

... command=lambda: [self._eng.download(self.mp3_var.get(), self.link_var.get()),
                     self.file_name.set(self._eng.download(self.mp3_var.get(), self.link_var.get())[0]),
                     self.file_path.set((self._eng.download(self.mp3_var.get(), self.link_var.get())[1]))])

There are 3 calls to self._eng.download, therefore the downloading will be triggered 3 times in a row. Luckily it seems that the YouTube library is smart enough to not re-download the same file.

I also don't understand why the lambda returns a list. It will never be used in this context.

It would be better to define a function to handle the download action, for example:

def _download(self):
    url = self.link_var.get()
    as_mp3 = self.mp3_var.get()

    filename, path = self._eng.download(as_mp3, url)

    self.file_name.set(filename)
    self.file_path.set(path)

and then when creating the button use command=self._download.

Note that it's intentional in Python that the lambda function can have only a single expression. The idea is that for anything even a little bit complex, it's recommended to create a dedicated function, and make it easy to read, rather than cramming complex logic into a one-liner.

Optimize for reading top to bottom

The snippet with the complex lambda in the previous section was actually very hard to spot, because the most important piece of code was in the far right, and I had to scroll horizontally to see it.

    download_button = ttk.Button(self, text='DOWNLOAD', style='TButton', command=lambda: [self._eng.download(self.mp3_var.get(), self.link_var.get()),
                                                                                          self.file_name.set(self._eng.download(self.mp3_var.get(), self.link_var.get())[0]),
                                                                                          self.file_path.set((self._eng.download(self.mp3_var.get(), self.link_var.get())[1]))])

At first, scrolling the code from top to bottom, I saw that most lines are just setting up UI stuff, not very interesting to review, and I almost didn't notice that there is something very important to see at the far right, in the complex lambda.

Make sure the most important parts of the code are easily visible, without horizontal scrolling. Also note that the Python style guide (PEP8) recommends maximum line length of 79 characters.

Validate inputs

The code doesn't validate the entered URL, simply passes it to YouTube constructor.

When the user enters an invalid URL (for example "foo", or blank), there is no visible response on the UI, and on the console there is a cryptic failure message coming from the YouTube library. This is not good UX.

It would be better to do at least some basic sanity checks on the input, and signal to the user when something looks wrong, rather than passing unverified values to a 3rd party API.

Naming

The Downloader.download takes as parameter MP3BoolVal... I think a more natural name would be audio_only. This is also consistent with the YouTube API method's naming (get_audio_only), focusing on a general purpose without mentioning a specific format. I suggest to do similarly in the App class too, and also rephrase the labels in the UI.

Answer 3

UI formatting

In APP.PY there is repeated stuff: background color, font style, form size and position etc. You could instead define constants for those. That makes it easier to change the running theme, something that the user could even fine-tune themselves, and even better, you could store settings to a configuration file with pickle or yaml for example.

It should be possible to override the download path too. It's not something that should be hardcoded in a class. The class should be flexible and portable. So the download path should be an argument to your __init__ but it is acceptable to provide a default fallback value.

String replace (file rename)

In the download class you have this:

name = name.replace('\\', '').replace('/', '')

If you had a more complex requirement you could use a regular expression to replace a range of characters, for example:

import re
name = re.sub('[\\\\|/]+', '', name)

Double escaping rules apply in this case because of the significance of the backslash - Discussion.

But this is not enough to have safe names: depending on the OS there are more characters that should be avoided, or that are bound to cause problems. Consider the following list. Excessive length can also cause problems, depending on the OS and the underlying file system.

Remember that any third-party input (in this case: the video title) is untrusted by definition. It has to be checked and validated in a way that is not going to disrupt your application, or worse induce security vulnerabilities. If you read up the literature about file uploads, the different attack techniques are explained, and some of them may apply to your situation.

So if you wanted to keep it "simple" you could adapt the regex to replace everything that is not alphanumeric + a few allowed characters like the hyphen or underscore. But this is still too simplistic. Some videos may have titles in an alphabet other than Latin.

And if you overwrite files without warning this is not good. Either show a confirmation message to the user, or add a suffix automatically eg .1 .2 etc.

Class

In the Downloader class, the sole method download does not even use self, so it could have been declared a static method right away (adding the @staticmethod decorator). But then the class does not achieve anything, it is merely a wrapper function around the YouTube API. It does not expose any meaningful methods otherwise.

The file rename could have been a class method for example. A static method in fact, because that function does not need the class instance either. Even though it is one-liner at present, it should be a bit more complex than that to be effective, as we have seen previously.

Likewise, handling of audio and video should be processed by distinct tasks. Thus the class would need to be broken down into smaller but specialized methods to become useful.

Ideally it should be possible to get real-time statistics about the state and speed of the download. For this purpose you have thread polling techniques or callback functions. Once you get a grasp on this, you can implement parallel download of multiple files without too much difficulty, like a Torrent client.

Transient content

For such an application it is customary to use temporary files, that are renamed at the end of the process, because the download can obviously fail for many reasons, or be cancelled by the user. As a result your Downloads directory is going to be clogged with useless files over time. Until you implement some kind of resume download feature, the aborted downloads are good for the trash bin. Have a look at the tempfile library. Some methods like NamedTemporaryFile can take care of automatic disposal of the file as soon as it is closed.

Validation etc

To be a more mature product your application should have some validation of user input, basic exception handling and a UI that includes a Cancel button, which involves some form of threading for the download to do it right. In Python this is not so difficult. I have not tested your application but I am wondering how responsive the UI is while processing a large download.

If you need to parse URLs Python has an interesting module: urllib.parse. Even without doing fancy stuff you can at least extract the scheme (http/https) and the domain name to ascertain that the URL looks minimally correct. Plus, it seems to handle IDNs like https://яндекс.рф/. A regex seems to be overkill.

Coding style

PEP8 one line per import, so:

import sys, os

becomes:

import sys
import os

What you are doing right: using the pathlib library for convenience and portability.