Page MenuHomePhabricator

Add chart functions "x()" and "constant(3)"
ClosedPublic

Authored by epriestley on Apr 17 2019, 2:04 PM.

Details

Summary

Depends on D20442. Ref T13279. Add basic support for drawing chart functions that are not based on Facts first-party ETL datasets. Some general goals:

  • This might be useful to draw a line like "goal" or "profitability".
  • This might be useful to pull data from an external source.
  • For composable functions like "add" or "subtract", which are useful in manipulating ETL datasets, these value functions will make testing easier.
Test Plan

Added a constant(256) function:

Diff Detail

Repository
rP Phabricator
Branch
chart7
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 22620
Build 31003: Run Core Tests
Build 31002: arc lint + arc unit

Event Timeline

epriestley created this revision.Apr 17 2019, 2:04 PM
epriestley requested review of this revision.Apr 17 2019, 2:06 PM
amckinley accepted this revision.Apr 17 2019, 11:16 PM
amckinley added inline comments.
src/applications/fact/chart/PhabricatorConstantChartFunction.php
30

Not sure how I feel about passing $limit here. Won't every chart function basically copy/paste the rest of the code in this function?

32–33

Also it's not obvious to me why $limit gets passed as an argument but the axis is an instance variable on this object.

src/applications/fact/controller/PhabricatorFactChartController.php
24

This is setting the default argument to 256, right? I was looking for some reason why this to be a power of 2, maybe pick a different constant like 42?

This revision is now accepted and ready to land.Apr 17 2019, 11:16 PM
epriestley added inline comments.Apr 18 2019, 8:15 PM
src/applications/fact/chart/PhabricatorConstantChartFunction.php
32–33

Yeah, this changes in the ~next diff.

src/applications/fact/controller/PhabricatorFactChartController.php
24

This is just drawing a visible line on the "demo" chart. 256 just happened to produce a reasonable line in the middle of the data on my local instance.

This revision was automatically updated to reflect the committed changes.