Page MenuHomePhabricator

PhamePostSearchEngine
ClosedPublic

Authored by chad on Jul 22 2015, 3:35 AM.
Tags
None
Referenced Files
F14057563: D13671.id33056.diff
Sun, Nov 17, 4:37 AM
F14050926: D13671.id33045.diff
Thu, Nov 14, 9:35 PM
F14014693: D13671.id33045.diff
Sun, Nov 3, 6:44 AM
F13991070: D13671.diff
Tue, Oct 22, 8:36 AM
F13990540: D13671.id.diff
Tue, Oct 22, 4:18 AM
F13978686: D13671.id33045.diff
Oct 19 2024, 12:29 AM
F13970801: D13671.id33056.diff
Oct 17 2024, 9:55 AM
F13964398: D13671.id33045.diff
Oct 15 2024, 8:13 PM
Subscribers

Details

Summary

A very basic search engine for Phame Posts

Test Plan

Built a dashboard to test various queries. Saw posts.

Diff Detail

Repository
rP Phabricator
Branch
phame-update-4
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/phame/query/PhamePostSearchEngine.php:63XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 7339
Build 7718: [Placeholder Plan] Wait for 30 Seconds
Build 7717: arc lint + arc unit

Event Timeline

chad retitled this revision from to PhamePostSearchEngine.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added reviewers: epriestley, btrahan.
epriestley edited edge metadata.

One inline -- does that work?

As a general philosophical thing which is really outside the scope of this change, APIs like $query->withStatuses($list_of_statuses) are generally better than APIs like $query->withStatus($status) because they're more flexible. Long ago we had some withStatus(...)-style APIs, and still have some vestigial versions of them in Differential and Maniphest I think, but users inevitably wanted things like "all open statuses", not just a single status. This lead to defining status constants so you'd do something like withStatus(BlahBlah::ANY_OPEN_STATUS), which was messy and not very general.

If I was writing this stuff today, I might do withVisibilities($list_of_values) instead of withVisibility($value) for this reason. As long as there are exactly two visibilities this doesn't really matter, but if we added a third or fourth state this UI might not be very good anymore (e.g., if we add a "retracted" or "scheduled to publish later" state, a single dropdown won't handle that very well, and storing a single integer as the saved storage value will require some glue to work with the new checkbox or typeahead control. It seems at least vaguely plausible that we'll add a "retracted" / "archived" sort of state in the future to let you throw away a draft that you decide not to move forward with.

For things with exactly two options that we're pretty sure will only ever have two options (e.g., "archived" on Projects), you can use PhabricatorSearchThreeStateField to build a "Don't Care / Yes / No" field, which is more flexible than a single checkbox.

I think this is basically fine as-implemented, though. If we do add more visibility states, the amount of glue we need is very small, and Phame is a prototype anyway.

src/applications/phame/query/PhamePostSearchEngine.php
21

Here (see note below).

63

To fix this, just do if (strlen($map['visibility'])) up above (see note), I think.

This revision is now accepted and ready to land.Jul 22 2015, 1:55 PM
This revision was automatically updated to reflect the committed changes.

I'll tackle that in a future diff, trying to make Phame updates more bite sized.