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
F19911882: D20445.diff
Thu, Apr 2, 3:36 PM
F19911881: D20445.diff
Thu, Apr 2, 3:36 PM
F19888154: D20445.id.diff
Mon, Mar 23, 10:37 PM
F19825277: D20445.id48829.diff
Sun, Mar 8, 9:57 AM
F19814252: D20445.id48753.diff
Mar 4 2026, 1:06 PM
F19811779: D20445.id48754.diff
Mar 4 2026, 2:30 AM
F19708007: D20445.id48829.diff
Feb 11 2026, 12:22 PM
F19517401: D20445.diff
Jan 15 2026, 10:47 AM
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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.