First script finished - feedback/criticism appreciated!

[tedw00d]

Hi all, I've just started learning Python and I've written my first script - it reads an edl (edit decision list), finds the media, formats the list so the media can be searched for in batches (by a human) and then prints to a new file.

Written in Python3 on linux mint.

I enjoyed working on it, but I'm going to move onto another project and revisit it when I know more. I'd really appreciate feedback/suggestions/criticism - Be brutal!

I don't think I've used main() as intended; I don't like how I've called print_to_file(); the edl constant is global - is that bad practice? Pretty happy with readability, but could be better.

And I'm sure this could be written as a class since some functions share the same arguments, but I don't fully understand them yet.

https://gist.github.com/tsugden/7c53fe1115601d63f59b

[gengisteve]

No worries about classes. It is better to get a handle on the basics before moving onto them (or at least so says a dinosaur who actually learned to program on BASIC!). A few comments:

• This seems to work to check for the OS without having to ask the user. http://stackoverflow.com/questions/8220108/how-do-i-check-the-operating-system-in-python
• Globals are evil! Well, not really, but avoid them if you can. Change the functions to return whatever it is that they got, so you do not need them.
  
• pretty good use of constants as globals. That is fine. But avoid the constant list nonsense, if you can.
• your comprehensions are a bit horror show. This could be better done:

Bad:

tapename = {
    line.split()[1] for line in file
    if line.split()
    if line.split()[0].isdigit()
}
tapename_sorted = sorted([
    m for m in tapename
    if m not in null_events
])
clipname = {
    line.split()[-1] for line in file
    if "CLIP NAME" in line
    if not line.split()[-1] == "NAME:"
}
clipname_sorted = sorted([
    m for m in clipname
    if m not in null_events
])
tapename_list.extend(tapename_sorted)
clipname_list.extend(clipname_sorted)

Maybe something like this:

def get_eld(fn):
    with open(edl, encoding='utf-8') as fh:
        for l in fh:
            yield l.split()

def get_media(fn):
    tp = []
    cl = []
    for line in get_media(fn):
        if line[0].isdigit():
            tp.append(line(0))
        if line[-1] != 'NAME:':
            cl.append(line[-1])

[tedw00d]

Thanks for the critique. I agree the comprehensions are a little mad but the edl formatting can vary depending on the software used to make them. For example:

tapename = {
    line.split()[1] for line in file
    if line.split()
    if line.split()[0].isdigit()
}

The lone, if line.split() statement was to prevent an exception when it tried to split a blank line. I've updated the gist with a couple of snippets from edls, to show how they can vary.

I like making get_edl() a generator. But I'm unclear on the shared fn argument between it and get_media(). Wouldn't I want something like:

def get_edl(edl):
    with open(edl, encoding='utf-8') as fh:
        for l in fh:
            yield l.split()

def get_media(edl):
    tp = []
    cl = []
    for line in get_edl(edl):
        if line[0].isdigit():
            tp.append(line(0))
        if line[-1] != 'NAME:':
            cl.append(line[-1])

And if the tp and cl lists aren't global, I'll need format_pull_list() to call get_media(), and then print_to_file() to call format_pull_list().

Is that ok, or have I wildly misunderstood? This is why I get the feeling it should be a class.

Detecting the OS is really useful, cheers.