Page MenuHomePhabricator

Get diviner to correctly handle contexts
Needs ReviewPublic

Authored by Firehed on Jul 1 2014, 9:54 PM.
Tags
None
Referenced Files
F13082797: D9791.diff
Wed, Apr 24, 10:17 PM
Unknown Object (File)
Sat, Apr 20, 7:12 PM
Unknown Object (File)
Fri, Apr 19, 6:35 PM
Unknown Object (File)
Fri, Apr 19, 6:35 PM
Unknown Object (File)
Fri, Apr 19, 6:35 PM
Unknown Object (File)
Fri, Apr 19, 6:07 PM
Unknown Object (File)
Fri, Apr 19, 5:42 PM
Unknown Object (File)
Mon, Apr 1, 10:55 PM

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T2284: Make Diviner namespace aware
Summary

Diviner has a largely-unused concept of "context" in order to handle namespaced code. This builds upon that to clean up some queries and rendering:

  • Contexts are displayed for classes, interfaces, and functions
  • Contexts are normalized during storage (replacing \ with :) so that URI routing works
  • URIs always include context in order to avoid some ambiguous routing
  • Sorting is a bit more logical

Ref T2284

Test Plan

Generated existing libphutil docs and saw stuff continued to work right. Generated docs for code with namespaced PHP code (commit coming separately) and observed that links for namespaced classes and functions work, look right, and that the linked pages function as expected.

Diff Detail

Repository
rP Phabricator
Branch
diviner
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 1655
Build 1656: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

Firehed retitled this revision from to Get diviner to correctly handle contexts.
Firehed updated this object.
Firehed edited the test plan for this revision. (Show Details)
Firehed added a reviewer: epriestley.

Some screenshots of this in action:

Screen_Shot_2014-07-01_at_2.55.58_PM.png (1×1 px, 261 KB)

Screen_Shot_2014-07-01_at_2.56.16_PM.png (1×2 px, 334 KB)

Screen_Shot_2014-07-01_at_2.55.22_PM.png (1×1 px, 295 KB)

The diff for the PHP integration still needs a little love (it handles namespace Foo; but not namespace Foo { }), but to test this, adding some setContext() calls in the atomizer with pretty much any random text is sufficient (note that it should include a trailing NS separator, unless we want to add language-specific logic in the renderer)

I think it's tentatively fine to put language-specific logic into the renderer. I think we need this logic per-language anyway for f(), a::b, o->m, x.y, etc. -- and the right way to join context on a method is context::method, not context\method (this might not ever actually happen in the UI right now), so this theoretically won't produce the right results unless we change how methods get atomized too.

In some future world where we support more than a handful of languages, I could imagine some pluggable class implementing these rules for each language without getting too gross, since there aren't a ton of them per language.

Let me play around with this a bit, I think it's within the ballpark of reasonable but we might be better off shuffling around where some parts happen.

Cool, thanks for the feedback!

There were a couple of areas that felt a little bit hairy especially when it came to rendering and normalization. Assuming "context" always means "fully qualified namespace" (incl. class a method is declared in) it's probably not too bad to abstract out differently. This stores the normalized NS in the diviner_livesymbol table (for search, etc) and the original variant in diviner_liveatom's json blob but the normalization routine would need work for other languages too. Unfortunately I don't think there's a good way to avoid the added query on the DivinerBookController (from needAtoms()) without altering the symbol table to store the language and then build out the switching logic.

Firehed edited edge metadata.
  • making diviner namespace aware
  • clean up a lot of the normalization and language-specific context separation logic
  • more cleanup
  • remove snippet from testing

After poking at this, I think we just need to add a displayContext field or similar across most of the stack. It would be helpful to rename title to displayName for consistency too, maybe.

The basic issue is that if we restore the "generate flat files on disk" generator, it can't emit backslashes (or some other special characters) to the filesystem and still work on Windows. We also need to deal with this fairly deep in the stack so we can handle symbols with name collisions appropriately (which we probably do an okay-but-not-completely-correct job on today). However, some of these transformations are destructive and there's no guarantee in the general case that we can restore the original value from the normalized value.

Let me take a crack at that...