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
F13050748: D20445.id48829.diff
Fri, Apr 19, 3:10 AM
F13050747: D20445.id48828.diff
Fri, Apr 19, 3:10 AM
F13050746: D20445.id48754.diff
Fri, Apr 19, 3:10 AM
F13050745: D20445.id48753.diff
Fri, Apr 19, 3:10 AM
F13050744: D20445.id.diff
Fri, Apr 19, 3:10 AM
Unknown Object (File)
Wed, Apr 17, 8:36 AM
Unknown Object (File)
Sun, Apr 14, 9:29 PM
Unknown Object (File)
Sun, Apr 14, 9:28 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 22623
Build 31008: Run Core Tests
Build 31007: 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.