Safely trash instead of rm

Question

This script is called trash and it goes on the path.

Usage: trash [OPTIONS] [TRASH_FILES]...

How clean/readable do you think this is? Is Python the right tool for the job?

#!/usr/bin/env python3
import os
import sys
import shutil
import itertools
from pathlib import Path
import click
first = next
def candidate_names(path):
 '''unused_names('f.txt') yields f.txt, f.txt_1, f.txt_2, ...'''
 yield path
 for i in itertools.count(1):
 yield path.with_name('{path.name}_{i}'.format(**locals()))
default_trash_dir = '~/.trash'
@click.command()
@click.argument('trash_files', nargs=-1)
@click.option('--trash-dir', default='', metavar='<trash_dir>', help=
'''destination for trash. Defaults to ${{TRASH_DIR}}, and then '{}'.'''
 .format(default_trash_dir))
def trash(trash_files, trash_dir):
 '''Safely trash the given files by moving them into a trash_dir.
Files are stored by their path relative to either ~ or / (preferring ~
if possible)
If an existing item with the same name is already in the trash, I
add _1, _2, ... to the end of the filename.
Metadata is copied; See Python documentation on shutil.copy2 for
details.
 \b
 $ touch a
 $ mkdir b
 $ touch b/c
 $ touch /tmp/d
 $ trash a b/c /tmp/d
 /home/sam/subdir/a -> /home/sam/.trash/subdir/a
 /home/sam/subdir/b/c -> /home/sam/.trash/subdir/b/c
 /tmp/d -> /home/sam/.trash/_root/tmp/d
 $ touch a
 $ trash a
 /home/sam/subdir/a -> /home/sam/.trash/subdir/a_1
 '''
 # get/make trashdir
 if not trash_dir:
 # empty-string and non-existence are treated the same way
 trash_dir = os.environ.get('TRASH_DIR', '')
 if not trash_dir:
 trash_dir = default_trash_dir
 trash_path = Path(trash_dir).expanduser()
 trash_path.mkdir(exist_ok=True, parents=True)
 for trash_file_ in trash_files:
 trash_file = Path(trash_file_)
 if not trash_file.exists():
 print("{}: not found".format(trash_file))
 sys.exit(1)
 # get absolute path
 trash_file = Path(trash_file_).resolve()
 # get a destination which does not already exist
 try:
 trash_dest_candidate = trash_path / trash_file.relative_to(Path.home())
 except ValueError:
 trash_dest_candidate = trash_path / '_root' / trash_file.relative_to('/')
 trash_dest = first(filter(lambda path: not path.exists(),
 candidate_names(trash_dest_candidate)))
 trash_dest.parent.mkdir(exist_ok=True, parents=True)
 print('{} -> {}'.format(trash_file, trash_dest))
 try:
 shutil.move(trash_file, trash_dest)
 except Exception as e:
 print(str(e))
 sys.exit(2)
if __name__ == '__main__':
 trash()

• nyvokub: Why reinvent the wheel? I believe it would be easier and best to make an alias for this. A great answer can be found here.

Answer 1

first = next

That needlessly pollutes the top-level namespace. Please bury that within def trash. Also, the first() call is odd -- it would be more idiomatic to simply call next() when obtaining first element.

 '''unused_names('f.txt') yields f.txt, f.txt_1, f.txt_2, ...'''

That's a code defect. The comment meant to refer to candidate_names().

You might choose to shift the responsibility, to only yield unused names, but the current code is fine, too.

 yield path.with_name('{path.name}_{i}'.format(**locals()))

Passing in **locals() isn't the most readable way. Please just phrase it as

 yield path.with_name(f'{path.name}_{i}')

Docstring:

Metadata is copied; See Python documentation on shutil.copy2 for details.

Kudos for delegating to official docs rather than copying a sentence from them. But it's probably better to point to move(), which itself points to copy2().

\b

Not sure what you meant by \b.

Maybe you don't need the metavar?

The trash_files can sometimes have long directory paths -- consider using SHA1 to compress the directory down to a short unique string.

if not trash_dir:

I don't understand why the click default wasn't set to os.environ.get('TRASH_DIR', '').

You have three expressions you want to probe here, call them d1, d2, d3. Rather than if / then, consider using or:

trash_dir = d1 or d2 or d3

Consider phrasing this

for trash_file_ in trash_files:

as

for trash_file in map(Path, trash_files):

I like the business of finding trash_dest_candidate relative to $HOME or /, whichever works. This could easily be a list of (more than 2) dirs.

 try:
 shutil.move(trash_file, trash_dest)
 except Exception as e:
 print(str(e))
 sys.exit(2)

Catching all exceptions doesn't seem to help you here -- might be better to not catch anything and live with the stack trace. The str() is superfluous. Consider calling exit() with errno from e, rather than constant 2. It appears to me that Ctrl-C is disabled during cross-filesystem file copies.

Overall, it looks like pretty good code that offers a nice service of "collapsing" filenames while avoiding collisions. Consider appending timestamp + pathname to a log file, and automatically pruning month-old files at end of each run, so trash does not grow without bound.