Page MenuHomePhabricator

Make chart function argument parsing modular/flexible with 900 pages of error messages
ClosedPublic

Authored by epriestley on Apr 17 2019, 6:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 28, 2:42 AM
Unknown Object (File)
Wed, Mar 27, 1:59 AM
Unknown Object (File)
Wed, Mar 27, 1:59 AM
Unknown Object (File)
Wed, Mar 27, 1:59 AM
Unknown Object (File)
Wed, Mar 27, 1:59 AM
Unknown Object (File)
Wed, Mar 27, 1:59 AM
Unknown Object (File)
Wed, Mar 27, 1:59 AM
Unknown Object (File)
Fri, Mar 22, 9:35 PM
Subscribers

Details

Summary

Depends on D20444. Ref T13279. Instead of ad-hoc parsing and messages, formalize chart function arguments.

Also, add a whole lot of extra type checking.

Test Plan

Built and charted various functions with various valid and invalid argument lists, got sensible-seeming errors and results.

Diff Detail

Repository
rP Phabricator
Branch
chart9
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22703
Build 31120: Run Core Tests
Build 31119: arc lint + arc unit

Event Timeline

  • Make the "constant" function actually read its argument.

This feature is really coming along, practically ready to ship!

Screen Shot 2019-04-17 at 11.44.11 AM.png (1×1 px, 217 KB)

amckinley added inline comments.
src/applications/fact/chart/PhabricatorChartFunctionArgumentParser.php
130

"requesting"

src/applications/fact/chart/PhabricatorXChartFunction.php
9

Just for the sake of code coverage, have you tried making any functions accept more than one argument? There's a lot of fancy looping/iterating in this diff that I don't think is getting put through its paces by the existing 0/1-argument functions.

This revision is now accepted and ready to land.Apr 18 2019, 11:37 PM

In a future diff, scale() and shift() take two arguments and work, although I'm likely rethinking exactly how the composition/chaining part of this works (see D20452 for rambling).

This revision was automatically updated to reflect the committed changes.