Page MenuHomePhabricator

Make FileFinder's withName() + withSuffix() work like every other Query class
ClosedPublic

Authored by epriestley on Jan 31 2018, 8:38 PM.
Tags
None
Referenced Files
F13905331: D18979.id45518.diff
Mon, Oct 7, 8:08 PM
F13905330: D18979.id45513.diff
Mon, Oct 7, 8:08 PM
F13905329: D18979.id.diff
Mon, Oct 7, 8:08 PM
Unknown Object (File)
Sat, Oct 5, 9:47 PM
Unknown Object (File)
Sat, Sep 21, 8:28 AM
Unknown Object (File)
Thu, Sep 12, 9:39 AM
Unknown Object (File)
Thu, Sep 12, 9:38 AM
Unknown Object (File)
Thu, Sep 12, 9:38 AM

Details

Summary

See PHI336. See https://discourse.phabricator-community.org/t/using-the-filefinder-class-withsuffix-and-withname-return-entries-with-either-the-suffix-or-the-name-match/1035?u=dangerouslyil.

Currently, withName() and withSuffix() mean "with either the name or the suffix", which isn't how any other Query class works.

Note that "name" means "exact name", per find semantics, so this combination isn't necesarily terribly useful. However, the current behavior is clearly surprising given that everything else in the project works differently.

Test Plan

Added a test case for both constraints, made it pass.

Diff Detail

Repository
rPHU libphutil
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

If you put a * in the ->withName pattern you should also be able to combine with suffix, i think?

src/filesystem/FileFinder.php
114

It won't in PHP mode, since this is just a straight string match here.

It may in find mode, but if so that's a bug since it would prevent you from matching a file that is literally named thing*.

We can add a constraint like withNameSubstring(<substring>) if that's what you're after, above and beyond this change?

The grand trick here is that this runs totally different code if find is available (Linux, macOS) or unavailable (Windows) so relying on find behavior isn't portable, and if we're exposing any find behavior that's a bug.

How much do you think I'm going to regret it if I commit a file named a-literal-star-* to the repository as a test case?

I think it's a worthwhile adventure to find out.

src/filesystem/__tests__/FileFinderTestCase.php
22–50

This is a really "special" way to write unit tests to make sure failures are impossible to understand and the tests are impossible to modify.

This revision is now accepted and ready to land.Jan 31 2018, 9:17 PM
This revision was automatically updated to reflect the committed changes.

@epriestley @yelirekim, to tighten up withName() to be an exact match for the find option, you would just need to preprocess the name string and escape special characters with a backslash (seems to be just *,?,[, and ] on both Mac and Linux).

I would like to post some improvements to this class, and could implement that as well.