Page MenuHomePhabricator

Inlines for custom herald actions
ClosedPublic

Authored by hach-que on Jul 2 2014, 4:43 AM.
Tags
None
Referenced Files
F14353338: D9796.id23526.diff
Thu, Dec 19, 4:34 PM
Unknown Object (File)
Mon, Dec 16, 6:26 PM
Unknown Object (File)
Sun, Dec 8, 5:55 PM
Unknown Object (File)
Sat, Dec 7, 4:53 AM
Unknown Object (File)
Fri, Dec 6, 6:23 PM
Unknown Object (File)
Fri, Dec 6, 2:38 PM
Unknown Object (File)
Mon, Dec 2, 10:43 AM
Unknown Object (File)
Tue, Nov 26, 7:50 PM

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Commits
Restricted Diffusion Commit
rP7baa0941b956: Inlines for custom herald actions
Summary

Ref D8784. Didn't see all of the inlines before hitting arc land. This fixes up the issues raised (and makes all the code nicer).

Test Plan

Made sure custom actions only appear for appropriate adapters and checked to ensure that they triggered correctly.

Diff Detail

Repository
rP Phabricator
Branch
herald-inlines
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1483
Build 1483: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

hach-que retitled this revision from to Inlines for custom herald actions.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.
hach-que edited edge metadata.
hach-que added inline comments.
src/applications/herald/adapter/HeraldAdapter.php
119

Not sure if this will throw on duplicates...

This revision should link to the previous one for context - or am I just not seeing it?

The previous diff has been landed; these are just some style changes.

Didn't see all of the inlines before hitting arc land. This fixes up the issues raised (and makes all the code nicer).

Reading this I have to ask myself: Which inlines? Which raised issues?

Hence, I would add a link to the landed Revision/Diff for *context*, so one can answer himself these questions.

If you still feel it would not be useful - so be it.

If an author doesn't provide a link, you can go to Differential > Advanced Search > Authors > "hach-que" to find stuff they've done recently, which turns up D9796 as the first hit.

@ite-klass Oh I was assuming since you were subscribed to the previous diff you would have seen it land (via your email or otherwise). It is D8784 for reference.

The important thing is that the functionality has landed; this diff doesn't change the functionality or behaviour, but just consists of some code styling changes.

hach-que edited edge metadata.

oh sorry my sleuthing is terrible

In fairness I went back to bed after that

epriestley edited edge metadata.

mpull() doesn't throw, per IRC. Everything else looks good.

This revision now requires changes to proceed.Jul 2 2014, 8:41 PM
hach-que edited edge metadata.

Ensure an exception is thrown on duplicate keys

epriestley edited edge metadata.

(Maybe put the offending key in the exception message too, makes it slightly easier to debug.)

This revision is now accepted and ready to land.Jul 3 2014, 1:52 AM
hach-que edited edge metadata.

Include key in exception message

hach-que updated this revision to Diff 23535.

Closed by commit rP7baa0941b956 (authored by @hach-que).

Yeah, *I*’m fine, but was thinking about others who may get confused (or me in a week). Sorry for the confusion.