design of Public API
The Path
routines were carefully designed to do just what they promise,
without unpleasant surprises.
This API assumes that caller wants either of a file or a tree
to be manipulated.
Fine, I will assume that's what caller wants.
The optional Path
annotations in the signatures are very nice.
When documenting such an API, your docstrings should
be pretty explicit about the fact that whole subtrees
maybe acted on, causing e.g. rm -rf
behavior.
As written, I find the use of "path" in an English sentence
to be a bit on the vague side.
My initial interpretation of "empty path" was Path("")
,
clearly different from Author's Intent.
OTOH a reference to "empty subdirectory" would have
immediately illuminated the situation.
The {src, target} identifiers are very nice, thank you.
behavior differs from a familiar library
On first reading I imagined that delete()
was roughly
df.unlink()
+
shutil.rmtree(df)
,
whichever was appropriate.
But upon reading it I realized that
there's no test suite which illustrates mkdir foo; date > foo/date.txt; ln -s foo bar; python -c '... delete("bar")'
.
As caller, I personally find it extremely surprising that
bar
got nuked (fine!), yet foo
and its contents continue to exist.
It seems pretty clear to me that a request to delete("bar")
should obliterate the pointer and thing(s) pointed at.
Oddly, the code won't do that.
It is perfectly fine to specify different behavior.
But you should be pretty clear when you do that,
drawing distinctions between the two libraries.
Then potential callers will understand the
advantages of your library for their particular use case.
meaningful identifiers
def delete(df: Path): ...
for dp, dn, fn in df.walk( ... ):
df
is part of your Public API, and so has a heavier
documentation burden than those three local variables.
Please use a more descriptive name such as target
or deleted_filespec
.
The three locals are fine, but I would have appreciated
root, directories, filenames
, or simply copying
dirpath, dirnames, filenames
straight out of the
man page.
In particular, I got hung up on "singular, singular, singular"
and was thinking we'd get one triple per file,
when of course the underlying API actually
returns "singular, plural_iterable, plural_iterable".
Short names can be fine for locals, but hopefully they
won't lead the Gentle Reader astray.
common lisp identifiers
(dp/f).unlink()
Please don't write it in that manner.
It looks like we're working with Scheme
or with the Common Lisp variable dp/f
,
after executing (defvar dp/f "foo")
.
When writing python code we surround /
slash and other
operators with whitespace.
Simplest way to accomplish that would be from the bash prompt:
$ black *.py.
clear parameter names
def link_dotfile(df: Path):
Apparently here I should read it as "dotfile" pathname,
rather than as "deleted file".
Please spell it out: dotfile: Path
.
Also, I found "onto" in the docstring confusing,
in the sense that I failed to understand this function's
contract.
I thought we would be creating a new link to an existing dotfile.
Turns out that we nuke a dotfile and then link out to a file
within some pip-installed package subtree.
Consider verifying that df.exists()
before doing anything drastic.
Arguably there's no need to verify that home_dotfile.exists()
,
since the delete()
call will just be a no-op.
Do consider tacking on a keyword default parameter
of nonexistant_ok=True
in its signature
so callers will understand that aspect.