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
F15392539: D20445.diff
Sat, Mar 15, 3:49 PM
F15385040: D20445.id48828.diff
Fri, Mar 14, 9:31 PM
F15379336: D20445.id48753.diff
Thu, Mar 13, 8:20 PM
F15315035: D20445.diff
Thu, Mar 6, 5:26 PM
F15284207: D20445.id48754.diff
Tue, Mar 4, 9:25 AM
Unknown Object (File)
Feb 16 2025, 12:00 PM
Unknown Object (File)
Feb 14 2025, 10:50 PM
Unknown Object (File)
Feb 14 2025, 10:50 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
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.