Page MenuHomePhabricator

Fixed localcommits include on getDiffDict
ClosedPublic

Authored by Pawka on Apr 14 2016, 2:59 PM.
Tags
None
Referenced Files
F14361224: D15710.diff
Fri, Dec 20, 11:06 AM
Unknown Object (File)
Wed, Dec 18, 12:28 AM
Unknown Object (File)
Sun, Dec 15, 1:59 PM
Unknown Object (File)
Tue, Dec 10, 7:11 AM
Unknown Object (File)
Wed, Dec 4, 7:16 PM
Unknown Object (File)
Thu, Nov 28, 11:58 AM
Unknown Object (File)
Tue, Nov 26, 2:33 AM
Unknown Object (File)
Sat, Nov 23, 10:01 AM
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
Branch
fix_localcommits
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 11720
Build 14679: arc lint + arc unit

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.