[PATCH] D111289: [LNT] Refactored the Graph page to use the library plotly instead of jQuery.flot

[Chris Matthews via Phabricator via llvm-commits]

cmatthews added a comment.

I just made a few comments while skimming this.



================
Comment at: lnt/server/ui/views.py:883
     # we want to load. Actually, we should just make this a single query.
-    #
-    # FIXME: Don't hard code field name.
-    values = session.query(plot_parameter.field.column, ts.Order.llvm_project_revision,
+    values = session.query(plot_parameter.field.column, ts.Order,
                            ts.Run.start_time, ts.Run.id) \
----------------
Line below needs indenting matched. Did this pass flake8?


================
Comment at: lnt/server/ui/views.py:1133
+                "yaxis": {"from": mean, "to": mean},
+                # "name": q_baseline[0].llvm_project_revision,
+                "name": "Baseline %s: %s (%s)" % (baseline_title, req.test.name, req.field.name),
----------------
Delete comment. 


================
Comment at: lnt/server/ui/views_util.py:15
+    from StringIO import StringIO as CsvStringIO  # for Python 2
+    from io import BytesIO as CsvBytesIO  # for Python 2
+except ImportError:
----------------
Why are you renaming StringIO to CsvStringIO, then writing json to it? Maybe keep this as StringIO.


================
Comment at: lnt/server/ui/views_util.py:20
+
+class BytesIOWrapper:
+    def __init__(self, string_buffer, encoding='utf-8'):
----------------
Need some docs. Why is this needed? Why not io.TextIOWrapper?


================
Comment at: lnt/server/ui/views_util.py:36
+
+def json_response(options, graph_plots, legend, revision_range, test_suite, baseline_plots):
+    json_obj = dict()
----------------
Doc comment here.


================
Comment at: lnt/server/ui/views_util.py:36
+
+def json_response(options, graph_plots, legend, revision_range, test_suite, baseline_plots):
+    json_obj = dict()
----------------
cmatthews wrote:
> Doc comment here.
everything is json in a web app. Better name?


================
Comment at: lnt/server/ui/views_util.py:61
+    else:
+        json_file = CsvBytesIO()
+        lines = flask_json.get_data()
----------------
What is this second code path for? these both return json?


================
Comment at: lnt/server/ui/views_util.py:71
+
+def graph_csv_response(options, num_plots, graph_parameters, graph_plots, mean_parameter):
+    def opt_is_true(o):
----------------
Docs.


================
Comment at: lnt/server/ui/views_util.py:71
+
+def graph_csv_response(options, num_plots, graph_parameters, graph_plots, mean_parameter):
+    def opt_is_true(o):
----------------
cmatthews wrote:
> Docs.
I'm confused by the return type of this. 


================
Comment at: lnt/server/ui/views_util.py:71
+
+def graph_csv_response(options, num_plots, graph_parameters, graph_plots, mean_parameter):
+    def opt_is_true(o):
----------------
cmatthews wrote:
> cmatthews wrote:
> > Docs.
> I'm confused by the return type of this. 
This needs some unit tests.


Repository:
  rLNT LNT

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111289/new/

https://reviews.llvm.org/D111289