Read total and detail in DirFileSystem when passed positionally

[Rodrigo-Palma]

The du, _du and find wrappers in DirFileSystem forward *args untouched to the wrapped filesystem, but read the flag they need for post-processing from kwargs alone:

def du(self, path, *args, **kwargs):
    total = kwargs.get("total", True)
    ret = self.fs.du(self._join(path), *args, **kwargs)

In the base signatures total is the first positional parameter after path (AbstractFileSystem.du(path, total=True, maxdepth=None, withdirs=False, **kwargs)) and detail is the fourth (AbstractFileSystem.find(path, maxdepth=None, withdirs=False, detail=False, **kwargs)). A caller who passes them positionally never reaches kwargs, so the wrapper takes its default branch and skips the conversion:

>>> dirfs.du("", total=False)
{"a.txt": 5, "sub/b.txt": 10}
>>> dirfs.du("", False)
{"/root/a.txt": 5, "/root/sub/b.txt": 10}   # absolute paths of the wrapped filesystem

>>> dirfs.find("", detail=True)
{"a.txt": {...}, "sub/b.txt": {...}}
>>> dirfs.find("", None, False, True)
["a.txt", "sub/b.txt"]                      # the detail mapping is dropped

Both fail silently, returning a well formed result that is wrong: the first leaks paths outside the directory the DirFileSystem is meant to scope to, which the class docstring states it assumes everything is relative to.

This binds each flag from kwargs first and from *args second, so the two call styles agree. _find and glob are left alone on purpose: neither accepts these flags positionally in the base class, so reading them from kwargs there is correct.

Two tests are added, each asserting that the positional call and the keyword call return the same thing.

[martindurant]

Or we could enforce kwarg-only?

[Rodrigo-Palma]

Good instinct on the index: _bind(kwargs, "detail", args, 2, False) reads the wrong argument, silently, the day a parameter is inserted before detail in the base signature. I have pushed a version that removes it.

I measured kwarg-only before going the other way, because it does have a real cost here:

• kwarg-only (def find(self, path, **kwargs)): 12 tests in test_dirfs.py fail with TypeError: DirFileSystem.find() takes 2 positional arguments but 4 were given. It also makes the wrapper stricter than what it wraps: AbstractFileSystem.find(path, maxdepth=None, withdirs=False, detail=False) accepts those positionally, so fs.find(path, None, False, True) would keep working on every filesystem except a wrapped one.
• Mirroring the base signature (what I pushed): positional calls keep working, the helper is gone, and nothing is bound by index.

So du and find now state their parameters and forward them by name.

That did mean touching the 12 tests, and it is worth saying why they were failing. They passed ARGS = ["foo", "bar"] through du and find as opaque filler and asserted it came out the other side untouched. Those values never corresponded to any real parameter of either method, so the tests were asserting the wrapper is a tunnel, which is the same assumption that let the bug through. They now pass the actual arguments and assert on those. The glob tests in the same file already do it this way, since glob has no *args to hide behind.

Full suite is green. Happy to switch to kwarg-only instead if you would rather have the stricter API and accept the divergence from the base, it is a small change from here.

[martindurant]

OK, then I suggest dropping this PR completely. It's the caller's responsibility to pass the right arguments - it's not typical in python to thoroughly check inputs anyway.

[Rodrigo-Palma]

Happy to close it. One thing first, in case it did not come across in the diff, because checking inputs is not what this changes.

The caller here is passing the right arguments. AbstractFileSystem.du(path, total=True, maxdepth=None, withdirs=False) takes total positionally, so du(path, False) is a valid call against the documented signature and it does the right thing on every filesystem. Wrapped:

>>> dirfs = DirFileSystem(path="/root", fs=mem)
>>> dirfs.du("", False)
{'/root/a.txt': 5, '/root/sub/b.txt': 10}
>>> dirfs.du("", total=False)
{'a.txt': 5, 'sub/b.txt': 10}

The root prefix leaks out of the wrapper on a correct call, with nothing raised. find loses detail the same way: dirfs.find("", None, False, True) returns ['a.txt', 'sub/b.txt'] instead of the detail dict.

So nothing is being validated and no input is rejected. The wrapper misreads one of its own arguments and hands back a different answer than the caller asked for, which is the part I was trying to fix.

Your call either way. Say the word and I will close it.

[martindurant]

Yes, I understood what it does, but it amounts to the same pattern. Correcting for the args order is like checking argument types, and brittle to changes, as you saw.

[Rodrigo-Palma]

Fair enough, closing.

One note for the record: the version I pushed does not correct for argument order. It states the parameters and lets Python bind them, which is what DirFileSystem.ls(self, path, detail=True, **kwargs) already does a few lines up in the same file. The index juggling you objected to was in the first commit and is gone.

That said, your point about drift is real: a mirrored signature can fall out of step with the base, and judging that cost is yours to make. Thanks for the quick turnarounds today.