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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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
5	There's no technical reason that the parent class needs to be abstract, although that is the normal use case.
96–98	Add ```lang=php?
103–109	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.