Paths

Table of Contentst

Differential D12946

Only link symbols if there might be any
ClosedPublic
Actions

Authored by avivey on May 20 2015, 6:24 AM.

Tags

None

Referenced Files

	Unknown Object (File)
	Thu, Dec 19, 10:04 AM

	Unknown Object (File)
	Thu, Dec 19, 9:36 AM

	Unknown Object (File)
	Thu, Dec 19, 7:13 AM

	Unknown Object (File)
	Sat, Dec 14, 1:14 PM

	Unknown Object (File)
	Fri, Dec 13, 8:51 PM

	Unknown Object (File)
	Fri, Dec 13, 8:52 AM

	Unknown Object (File)
	Sun, Dec 8, 1:51 AM

	Unknown Object (File)
	Fri, Dec 6, 12:30 PM

Subscribers

Details

Reviewers

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T8260: Differential Repositories are linking to non-existent symbols

Commits

Restricted Diffusion Commit
rPf21972a01f1a: Only link symbols if there might be any

Summary

fixes T8260. Only turn on symbol links if:

The repository has any configuration about symbols, or
There actually are symbols in the repository.

Test Plan

Look at revisions and files in various states of configurations and having symbols.

Diff Detail

Repository

Branch

arcpatch-D12946

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 6165
Build 6186: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

avivey updated this revision to Diff 31182.May 20 2015, 6:24 AM

avivey retitled this revision from to Only link symbols if there might be any.

avivey updated this object.

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

avivey added a reviewer: epriestley.

avivey added a task: T8260: Differential Repositories are linking to non-existent symbols.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 20 2015, 6:24 AM

Herald added subscribers: epriestley, Korvin. · View Herald Transcript

avivey added inline comments.May 20 2015, 6:31 AM

src/applications/differential/controller/DifferentialRevisionViewController.php
309–317	I was somewhat surprised to learn that this doesn't do anything; Actual active code is in `php DifferentialChangesetDetailView`. Without this section, symbols keep working even for code in comments.

joshuaspence added a subscriber: joshuaspence.May 20 2015, 8:10 AM

One stray var_dump().

src/applications/diffusion/controller/DiffusionBrowseFileController.php
279	When testing arrays for emptiness, prefer `if ($x)` over `if (empty($x))` for consistency and simplicity.
286	Debugging `var_dump()`.

This revision now requires changes to proceed.May 20 2015, 1:56 PM

s/empty($x)/$x

src/applications/diffusion/query/DiffusionSymbolQuery.php
133	Mind if I keep this `empty()`, to effectively cast this to boolean?

epriestley accepted this revision.May 21 2015, 3:25 PM

epriestley edited edge metadata.

This revision is now accepted and ready to land.May 21 2015, 3:25 PM

Closed by commit rPf21972a01f1a: Only link symbols if there might be any (authored by avivey, committed by epriestley). · Explain WhyMay 21 2015, 3:25 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

src/

applications/

differential/

controller/

DifferentialRevisionViewController.php

27 lines

diffusion/

controller/

DiffusionBrowseFileController.php

19 lines

query/

DiffusionSymbolQuery.php

20 lines

Diff 31208

src/applications/differential/controller/DifferentialRevisionViewController.php

Loading...

src/applications/diffusion/controller/DiffusionBrowseFileController.php

Loading...

src/applications/diffusion/query/DiffusionSymbolQuery.php

Loading...