2
\$\begingroup\$

This code works, but I feel bad about it. Did I achieve Python Zen or create a terrible monster? Note: it doesn't have to be super-fast or work on super-huge directory trees.

import glob
from typing import List

# Provide pathname without a trailing /
def find_files_with_extension(pathname: str, extension: str) -> List[str]:
    return list({item for sublist in [glob.iglob(s + "**." + extension) for s in glob.iglob(pathname + "/**/", recursive=True)] for item in sublist})
\$\endgroup\$
4
\$\begingroup\$

A few suggestions:

  1. This doesn't need to be a one line function. Writing it on one line makes it much harder to understand and makes finding mistakes more difficult.
  2. Use os.path.join to assemble your full pathname + wildcard + extension. That way, you don't need to worry about whether or not your pathname is given with or without a trailing /
  3. If you're going to worry about a trailing / in your pathname, you should be equally worried about a preceding . in your extension.
  4. glob.glob already returns a list with the matching files. I don't see any need for the complicated, nested list comprehensions you're using. Your function should essentially be a wrapper for calling glob.glob that simply makes combining the pathname + extension a little easier for the user.

That being said, here's how I might re-write this function:

import glob
import os

from typing import List

def find_files_with_extension(path_name: str, extension: str) -> List[str]:
    extension_wildcard = "*"

    if extension[0] != ".": # suggestion #3
        extension_wildcard += "."
    extension_wildcard += extension

    path_name_wildcard = os.path.join(path_name, "**", extension_wildcard) # suggestion #2

    return glob.glob(path_name_wildcard, recursive=True)
\$\endgroup\$
2
  • \$\begingroup\$ The hardest thing about this is you didn't actually test your code! Python uses # for comments not //. But it's better than mine. +1 \$\endgroup\$ – OrgnlDave Oct 8 '20 at 13:23
  • \$\begingroup\$ Apologies, I wrote and tested it without comments, then added them in before I posted! \$\endgroup\$ – theasianpianist Oct 8 '20 at 18:46
1
\$\begingroup\$

Here's a solution which should work fine, using the new, convenient pathlib:

import pathlib


def find_files_with_extension(path_name, extension):
    return list(pathlib.Path(path_name).rglob("*." + extension))
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for this! I wonder how many more years it will take for Python teachers to teach pathlib instead of os.path. However, you should not return a list. rglob already returns an iterable. \$\endgroup\$ – Zababa Oct 13 '20 at 0:11
  • \$\begingroup\$ @Zababa I don't mind if teachers are slow to adapt, I just wish more people here (and on SO) made use of it! \$\endgroup\$ – AMC Oct 13 '20 at 0:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.