Page MenuHomePhabricator

Improve parsing of docblock specials
ClosedPublic

Authored by joshuaspence on Aug 23 2016, 12:26 AM.
Tags
None
Referenced Files
F14391642: D16431.diff
Sat, Dec 21, 8:08 PM
Unknown Object (File)
Thu, Dec 19, 2:27 PM
Unknown Object (File)
Thu, Dec 19, 3:51 AM
Unknown Object (File)
Mon, Dec 16, 3:27 PM
Unknown Object (File)
Fri, Dec 13, 4:32 AM
Unknown Object (File)
Sat, Dec 7, 11:05 AM
Unknown Object (File)
Sat, Dec 7, 8:58 AM
Unknown Object (File)
Sat, Dec 7, 7:08 AM
Subscribers

Details

Summary

Currently the tags of following docblock are parsed as array('special' => "foo\nbar"). This diff changes the output to be array('special' => array('foo', 'bar')).

/**
 * @special foo
 * @special bar
 */
Test Plan

Ran unit tests.

Diff Detail

Repository
rPHU libphutil
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 13392
Build 17191: Run Core Tests
Build 17190: arc lint + arc unit

Event Timeline

joshuaspence retitled this revision from to Improve parsing of docblock specials.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

One inline -- and what does this produce (is there a test for it)?

@key value
@key
src/parser/PhutilDocblockParser.php
121

This seems a little odd to me, I'd expect:

@morse-code dot
@morse-code dot
@morse-code dash

...or whatever to produce array('dot', 'dot', 'dash'). Is there a particular reason for it?

src/parser/PhutilDocblockParser.php
121

Right, this was primarily intended for this case:

@special
@special
@special

In this case, I figured true was better than array(true, true, true).

joshuaspence edited edge metadata.

Add another test case

Ah, that makes sense for the no-strings case. Agreed that true is probably the right value there.

(I'm actually not really sure what I expect string + no string case to do.)

epriestley edited edge metadata.

Cool, those behaviors seem reasonable to me. Thanks!

(Does @stable @stable @stable have a test case?)

This revision is now accepted and ready to land.Aug 23 2016, 2:01 AM
joshuaspence edited edge metadata.

Add another test case

This revision was automatically updated to reflect the committed changes.