Lift class map construction into a new class map query
ClosedPublic
Actions

Authored by epriestley on Jun 22 2015, 4:44 PM.

Details

Reviewers

joshuaspence
btrahan

Maniphest Tasks

T8637: Maniphest batch editor can affect far too many tasks

Commits

rPHU74c9cb3a266e: Lift class map construction into a new class map query

Summary

Ref T8637. The "load concrete subclasses" pattern has enjoyed wide success, but we're starting to collect a lot of instances of "loop over this list of things and make sure they all really have unique keys".

We can better support this, caching, subclass expansion, and sorting in a thin layer on top of PhutilSymbolLoader.

Test Plan

See next diff.

Diff Detail

Repository

rPHU libphutil

Branch

classmap

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 6939
Build 6961: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley updated this revision to Diff 32411.Jun 22 2015, 4:44 PM

epriestley retitled this revision from to Lift class map construction into a new class map query.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added reviewers: btrahan, joshuaspence.

epriestley added a task: T8637: Maniphest batch editor can affect far too many tasks.

Herald added a subscriber: epriestley. · View Herald TranscriptJun 22 2015, 4:44 PM

btrahan accepted this revision.Jun 22 2015, 9:11 PM

btrahan edited edge metadata.

This revision is now accepted and ready to land.Jun 22 2015, 9:11 PM

This should maybe have some unit tests?

src/symbols/PhutilClassMapQuery.php
4	There's no technical reason that the parent class needs to be abstract, although that is the normal use case.
95–97	Add ```lang=php?
102–108	As above

I think we should definitely get code blocks inside source code documentation defaulting to their source file language per T5323 as a solution for the lang=php thing.

Closed by commit rPHU74c9cb3a266e: Lift class map construction into a new class map query (authored by epriestley, committed by epriestley). · Explain WhyJun 23 2015, 5:21 PM

This revision was automatically updated to reflect the committed changes.

I also don't see an easy way to unit test this without building two (three? four?) separate class trees for each major case. I figure failures should be pretty obvious from usage and other getAll() tests.