All of Python users should be familiar with Zen.
In this case I can see violation of next rules
Readability counts.
given example is too hard to reason about.
Simple is better than complex.
Sparse is better than dense.
there are too many statements in one comprehension.
Flat is better than nested.
and we've got nested comprehensions here.
Overview
side effects inside of comprehensions is not a good idea
x[1].remove(skip_dir)
see discussion here
instead of string concatenation
x[0].replace('\\', '/') + "/" + f
we use os.path.join
os.path.join(x[0].replace('\\', '/'), f)
about statement
for x in os.walk(".")
os.walk
yields tuples with names of root directory, subdirectories and files, so it will be better to unpack tuple instead of accessing coordinates by index
for root, directories_names, files_names in os.walk('.')
if we are using the same regular expression many times then it deserves to be compiled before using, so
... re.sub(r"^.*?FStar\.(.*)\.fs(.*)", dest_dir + r"Fstar.\1.fst\2", s) ...
can be separated into
TARGET_FILES_NAMES_RE = re.compile(r"^.*?FStar\.(.*)\.fs(.*)")
... TARGET_FILES_NAMES_RE.sub(dest_dir + r"Fstar.\1.fst\2", s) ...
also it is unlear what should dest_dir + r"Fstar.\1.fst\2"
do: i believe that it should join dest_dir
to simplified file name.
Main idea
When comprehension becomes complex (or especially complicated) it is a good idea to rewrite it into generator function.
For given example it may be like
TARGET_FILES_NAMES_RE = re.compile(r"^.*?FStar\.(.*)\.fs(.*)")
def modify_paths(top, dest_dir, skip_dir, pattern):
replacement = os.path.join(dest_dir, r"Fstar.\1.fst\2")
for root, directories_names, files_names in os.walk(top):
try:
# we are removing `skip_dir` from all subdirectories,
# is it a desired behavior?
directories_names.remove(skip_dir)
except ValueError:
# not in list
pass
for file_name in files_names:
if not fnmatch(file_name, pattern):
continue
s = os.path.join(root.replace('\\', '/'), file_name)
yield (s,
TARGET_FILES_NAMES_RE.sub(replacement, s))
but it is still raw and should be refactored.
Test
I've created directory
/
doc.txt
FStar.anotherfs
FStar.other.fS
FStar.some.fs90
FStar.text..fs
test.py
skip_me/
FStar.file.fs
FStar.sample.fs
FStar.skipped.fs
where test.py
contents:
import os
import re
from fnmatch import fnmatch
TARGET_FILES_NAMES_RE = re.compile(r"^.*?FStar\.(.*)\.fs(.*)")
def modify_paths(top, dest_dir, skip_dir, pattern):
replacement = os.path.join(dest_dir, r"Fstar.\1.fst\2")
for root, directories_names, files_names in os.walk(top):
try:
# we are removing `skip_dir` from all subdirectories,
# is it a desired behavior?
directories_names.remove(skip_dir)
except ValueError:
# not in list
pass
for file_name in files_names:
if not fnmatch(file_name, pattern):
continue
s = os.path.join(root.replace('\\', '/'), file_name)
yield (s,
TARGET_FILES_NAMES_RE.sub(replacement, s))
if __name__ == '__main__':
top = '.'
skip_dir = 'skip_me'
patt = '*'
# slash is required for version with list comprehension
dest_dir = 'dest_dir/'
before = [
(s,
re.sub(r"^.*?FStar\.(.*)\.fs(.*)", dest_dir + r"Fstar.\1.fst\2", s))
for s in (os.path.join(x[0].replace('\\', '/'), f)
for x in os.walk(top)
if (skip_dir in x[1] and
x[1].remove(skip_dir) or True)
for f in x[2] if fnmatch(f, patt))]
after = list(
modify_paths(top=top, dest_dir=dest_dir, skip_dir=skip_dir,
pattern=patt))
assert after == before
and assertion is successful.
P. S.
Skipping exceptions is not a good idea but if you know what to expect it can become a powerful tool.
Also it can be re-written using contextlib.suppress
context manager from
try:
directories_names.remove(skip_dir)
except ValueError:
pass
to
with suppress(ValueError):
directories_names.remove(skip_dir)