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.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Apr 17 2019, 6:29 PM
epriestley requested review of this revision.Apr 17 2019, 6:30 PM
epriestley updated this revision to Diff 48754.Apr 17 2019, 6:34 PM
  • Make the "constant" function actually read its argument.

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

amckinley accepted this revision.Apr 18 2019, 11:37 PM
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).

epriestley updated this revision to Diff 48828.Apr 23 2019, 7:21 PM
  • Typo fix.
This revision was automatically updated to reflect the committed changes.