Page MenuHomePhabricator

Fixed localcommits include on getDiffDict
ClosedPublic

Authored by Pawka on Apr 14 2016, 2:59 PM.
Tags
None
Referenced Files
F13059568: D15710.diff
Fri, Apr 19, 4:29 PM
Unknown Object (File)
Thu, Apr 11, 8:21 AM
Unknown Object (File)
Sat, Apr 6, 10:09 AM
Unknown Object (File)
Sat, Apr 6, 6:04 AM
Unknown Object (File)
Mon, Apr 1, 3:59 PM
Unknown Object (File)
Thu, Mar 28, 9:06 PM
Unknown Object (File)
Thu, Mar 21, 4:40 PM
Unknown Object (File)
Thu, Mar 21, 4:40 PM
Subscribers

Details

Summary

Ref T10808

Test Plan

Call differential.querydiffs method and expect 'local:commits' property be added to the result.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Pawka retitled this revision from to Fixed localcommits include on getDiffDict.
Pawka updated this object.
Pawka edited the test plan for this revision. (Show Details)
Pawka added a reviewer: epriestley.

Because

>>> $a = ['hello' => []];
=> [
       "hello" => []
   ]
>>> $b = ['hello' => [1, 2, 3]];
=> [
       "hello" => [
           1,
           2,
           3
       ]
   ]
>>> $a + $b;
=> [
       "hello" => []
   ]
>>> array_merge($a, $b);
=> [
       "hello" => [
           1,
           2,
           3
       ]
   ]
epriestley edited edge metadata.

As written, I think this is not correct, since array_merge($a + $b) should be the same as $a + $b. You probably want array_merge($a, $b) (that is, $..., $..., not $... + $...)

array_merge() has surprising behavior when arrays contain numeric keys. I'd prefer this continue to use +, but be restructured something like this:

$dict = array(...);
$authorship = ...
$defaults = array('properties' => array());

return $dict + $authorship + $defaults;

Or just make getDiffAuthorshipDict() always return a (possibly empty) properties, and remove it from $dict.

This revision now requires changes to proceed.Apr 14 2016, 3:04 PM
Pawka edited edge metadata.

Fix both left hands to become one left and one right.

epriestley edited edge metadata.
This revision is now accepted and ready to land.Apr 15 2016, 1:30 PM

Thanks! I added you to Blessed Committers so you should be able to land this (or "Land Revision") this yourself. See that project page for some instructions if you run into trouble.

(I also added you to Community, so you have a broader range of editing powers now.)

Closed by commit rPf05c3e41b9c4: Fixed localcommits include on getDiffDict (authored by Povilas Balzaravicius Pawka <povilas@uber.com>). · Explain WhyApr 15 2016, 3:06 PM
This revision was automatically updated to reflect the committed changes.