Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797043002:
Add a presubmit checker for duplicated enum values in UseCounter::Feature (Closed)
DescriptionAdd a presubmit checker for duplicated enum values in UseCounter::Feature
BUG=577772
Review-Url: https://codereview.chromium.org/2797043002
Cr-Commit-Position: refs/heads/master@{#462591}
Committed: https://chromium.googlesource.com/chromium/src/+/b84a182dfd565ca8080e0c51dd181f88a0a9452e
Patch Set 1 #
Total comments: 2
Patch Set 2 : Codereview: nit #
Total comments: 1
Patch Set 3 : Codereview: nit #
Total comments: 4
Patch Set 4 : Nit: fix a typo in the comment #
Messages
Total messages: 42 (27 generated)
Description was changed from ========== Add a presubmit checker for duplicated enum values UseCounter::Feature BUG=577772 TBR=rbyres@chromium.org ========== to ========== Add a presubmit checker for duplicated enum values UseCounter::Feature BUG=577772 TBR=rbyres@chromiumg.org ==========
Hi Rick, could you please take a look at the change I made in the presubmit checker for UseCounter? Thanks
I have verified that the checker works locally
Description was changed from ========== Add a presubmit checker for duplicated enum values UseCounter::Feature BUG=577772 TBR=rbyres@chromiumg.org ========== to ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 TBR=rbyres@chromiumg.org ==========
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM (Use R= instead of TBR= for CLs that need review.)
Description was changed from ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 TBR=rbyres@chromiumg.org ========== to ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 R=rbyres@chromiumg.org ==========
rbyers@chromium.org changed reviewers: + rbyers@chromium.org
LGTM with nits, thanks! https://codereview.chromium.org/2797043002/diff/1/tools/metrics/histograms/up... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2797043002/diff/1/tools/metrics/histograms/up... tools/metrics/histograms/update_histogram_enum.py:41: """Returns a dictionary of enum values, read from a C++ file. nit: update this comment to reflect the new return value format https://codereview.chromium.org/2797043002/diff/1/tools/metrics/histograms/up... tools/metrics/histograms/update_histogram_enum.py:78: # If two enum constraints have the same value s/constraints/constants/ ?
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2797043002/#ps20001 (title: "Codereview: nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 R=rbyres@chromiumg.org ========== to ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 ==========
lunalu@chromium.org changed reviewers: + dpranke@google.com
Hi dpranke, could you please owner lgtm tools/metrics/histograms/update_histogram_enum.py Thanks
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm https://codereview.chromium.org/2797043002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2797043002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:190: return False, duplicated_values Nit: if duplicated_values: return False, duplicated_values
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, haraken@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2797043002/#ps40001 (title: "Codereview: nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491493210024400,
"parent_rev": "10e9c6e5598d51459d77c5055d93c5ee4d53c870", "commit_rev":
"b84a182dfd565ca8080e0c51dd181f88a0a9452e"}
Message was sent while issue was closed.
Description was changed from ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 ========== to ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 Review-Url: https://codereview.chromium.org/2797043002 Cr-Commit-Position: refs/heads/master@{#462591} Committed: https://chromium.googlesource.com/chromium/src/+/b84a182dfd565ca8080e0c51dd18... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b84a182dfd565ca8080e0c51dd18...
Message was sent while issue was closed.
isherman@chromium.org changed reviewers: + isherman@chromium.org
Message was sent while issue was closed.
Hmm, it looks like nobody from the metrics team reviewed this CL... https://codereview.chromium.org/2797043002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/PRESUBMIT.py (right): https://codereview.chromium.org/2797043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/PRESUBMIT.py:77: should_update_histogram, duplicated_values = update_histogram_enum.HistogramNeedsUpdate( Should this wrap to 80-col? https://codereview.chromium.org/2797043002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2797043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:41: """Returns a dictionary of enum values and a pair of labels that have the same Please clarify this comment. The function returns a pair of labels, or None if there are no such elements. (It's probably clearer to write that as two sentences, though.) https://codereview.chromium.org/2797043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:81: return result, (result[enum_value], label) Hmm, this is an early return. Should the result be None if there is an error? In general, I wonder whether it makes more sense to implement this by throwing an exception, rather than returning a second result value. https://codereview.chromium.org/2797043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:177: changed. Please update the documentation around the return value (or, perhaps, the thrown exception).
Message was sent while issue was closed.
Thanks for doing this. I noticed this bug the other day but haven't got a chance to fix it.
Message was sent while issue was closed.
Hi Ilya, Thank you for the comments. I made some changes to the checker. Could you PTAL?
Message was sent while issue was closed.
Description was changed from ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 Review-Url: https://codereview.chromium.org/2797043002 Cr-Commit-Position: refs/heads/master@{#462591} Committed: https://chromium.googlesource.com/chromium/src/+/b84a182dfd565ca8080e0c51dd18... ========== to ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 Review-Url: https://codereview.chromium.org/2797043002 Cr-Commit-Position: refs/heads/master@{#462591} Committed: https://chromium.googlesource.com/chromium/src/+/b84a182dfd565ca8080e0c51dd18... ==========
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Description was changed from ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 Review-Url: https://codereview.chromium.org/2797043002 Cr-Commit-Position: refs/heads/master@{#462591} Committed: https://chromium.googlesource.com/chromium/src/+/b84a182dfd565ca8080e0c51dd18... ========== to ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772,711205 Review-Url: https://codereview.chromium.org/2797043002 Cr-Commit-Position: refs/heads/master@{#462591} Committed: https://chromium.googlesource.com/chromium/src/+/b84a182dfd565ca8080e0c51dd18... ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/19 22:05:28, loonybear wrote: > Hi Ilya, > > Thank you for the comments. I made some changes to the checker. Could you PTAL? A CL associated with this issue has already been committed. Could you please upload a new CL, and restore the previous CL description? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:60001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772,711205 Review-Url: https://codereview.chromium.org/2797043002 Cr-Commit-Position: refs/heads/master@{#462591} Committed: https://chromium.googlesource.com/chromium/src/+/b84a182dfd565ca8080e0c51dd18... ========== to ========== Add a presubmit checker for duplicated enum values in UseCounter::Feature BUG=577772 Review-Url: https://codereview.chromium.org/2797043002 Cr-Commit-Position: refs/heads/master@{#462591} Committed: https://chromium.googlesource.com/chromium/src/+/b84a182dfd565ca8080e0c51dd18... ==========
Message was sent while issue was closed.
FYI - I believe that this CL broke presubmits from tools/metrics/histograms/presubmit_bad_message_reasons.py and tools/metrics/histograms/presubmit_scheme_histograms.py. I am proposing a fix at https://codereview.chromium.org/2841823007. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
