Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(29)

Issue 2620713002: Factored stdio parsing out of graph_json. (Closed)

Created:
3 years, 11 months ago by sullivan
Modified:
3 years, 11 months ago
Reviewers:
eakuefner
CC:
catapult-reviews_chromium.org, perf-dashboard-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

Factored stdio parsing out of graph_json. These functions will be used in a future CL to show debugging info for stoppage alerts. Also added additional testing. BUG=catapult:#3132 Review-Url: https://codereview.chromium.org/2620713002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/0c9efe760d2cb739ac5cc81463961afd0a4c710b

Patch Set 1 #

Patch Set 2 : Use same links in tests as in datastore #

Patch Set 3 : fix tests #

Patch Set 4 : fix more typos in unit tests #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+103 lines, -20 lines) Patch
M dashboard/dashboard/common/utils.py View 2 chunks +40 lines, -0 lines 4 comments Download
M dashboard/dashboard/common/utils_test.py View 1 2 3 1 chunk +53 lines, -0 lines 0 comments Download
M dashboard/dashboard/graph_json.py View 2 chunks +10 lines, -20 lines 0 comments Download

Messages

Total messages: 27 (21 generated)
sullivan
3 years, 11 months ago (2017-01-09 15:54:32 UTC) #4
eakuefner
lgtm https://codereview.chromium.org/2620713002/diff/60001/dashboard/dashboard/common/utils.py File dashboard/dashboard/common/utils.py (right): https://codereview.chromium.org/2620713002/diff/60001/dashboard/dashboard/common/utils.py#newcode471 dashboard/dashboard/common/utils.py:471: no_details = (None, None, None, None, None) I'm ...
3 years, 11 months ago (2017-01-09 21:45:19 UTC) #19
eakuefner
Would also like BUG= here.
3 years, 11 months ago (2017-01-09 21:46:20 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2620713002/60001
3 years, 11 months ago (2017-01-09 22:12:44 UTC) #23
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/0c9efe760d2cb739ac5cc81463961afd0a4c710b
3 years, 11 months ago (2017-01-09 22:14:24 UTC) #26
sullivan
3 years, 11 months ago (2017-01-09 22:20:36 UTC) #27
Message was sent while issue was closed.
https://codereview.chromium.org/2620713002/diff/60001/dashboard/dashboard/com...
File dashboard/dashboard/common/utils.py (right):

https://codereview.chromium.org/2620713002/diff/60001/dashboard/dashboard/com...
dashboard/dashboard/common/utils.py:471: no_details = (None, None, None, None,
None)
On 2017/01/09 21:45:19, eakuefner wrote:
> I'm a little worried about using such a large tuple because of the potential
for
> confusion. Is it worth using a POD instead? Up to you.

I think it's okay because this method is mostly used by the two methods below.

https://codereview.chromium.org/2620713002/diff/60001/dashboard/dashboard/com...
dashboard/dashboard/common/utils.py:503: bot = re.sub(r'[ \(\)]', '_', bot)
On 2017/01/09 21:45:19, eakuefner wrote:
> Is this needed because it's a substitution that Logdog makes? Otherwise,
> urllib.quote will already urlencode the parens and spaces.

Yes, exactly, logdog does totally different substitution than url encoding.

Powered by Google App Engine
This is Rietveld 408576698