Paths

Table of Contentst

Differential D18979

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

Authored by epriestley on Jan 31 2018, 8:38 PM.

Details

Reviewers

amckinley

Commits

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

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

Branch

finder1

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 19302
Build 26086: Run Core Tests
Build 26085: arc lint + arc unit

Event Timeline

epriestley created this revision.Jan 31 2018, 8:38 PM

Harbormaster completed remote builds in B19302: Diff 45513.Jan 31 2018, 8:38 PM

epriestley requested review of this revision.Jan 31 2018, 8:38 PM

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

epriestley added inline comments.Jan 31 2018, 8:45 PM

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.

epriestley added inline comments.Jan 31 2018, 8:59 PM

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.

epriestley added a child revision: D18980: Rewrite FileFinder tests to actually show why they break.Jan 31 2018, 9:13 PM

amckinley accepted this revision.Jan 31 2018, 9:17 PM

This revision is now accepted and ready to land.Jan 31 2018, 9:17 PM

Closed by commit rPHU65e1147fd572: Make FileFinder's withName() + withSuffix() work like every other Query class (authored by epriestley). · Explain WhyJan 31 2018, 10:43 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.

Revision Contents
Changeset List

Path

Size

src/

filesystem/

FileFinder.php

66 lines

__tests__/

FileFinderTestCase.php

16 lines

Diff 45513

View Options

src/filesystem/FileFinder.php

View Options

src/filesystem/tests/FileFinderTestCase.php

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