Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2256773004:
Add more informtion to the CSV generated (Closed)
DescriptionAdd more informtion to the CSV generated
Add CSV row headers, days since last comment and whether or not bug is closed and owner to the CSV
BUG=597797
Committed: https://crrev.com/c3e861ceca16f92cf5eb0f889d208175bc971d3b
Cr-Commit-Position: refs/heads/master@{#413664}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Refactors #Patch Set 3 : Add owner name - will be needed later #
Total comments: 10
Patch Set 4 : Ready to land #Messages
Total messages: 25 (14 generated)
The CQ bit was checked by nainar@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...
nainar@chromium.org changed reviewers: + qyearsley@chromium.org
Hi, I added some of the info you wanted to the CSV. PTAL? Thanks! https://codereview.chromium.org/2256773004/diff/1/test.csv File test.csv (right): https://codereview.chromium.org/2256773004/diff/1/test.csv#newcode1 test.csv:1: crbug link, test file, days since last updated, status here as a sample of the csv generated
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM I also have some suggestions for possible follow-ups :-) https://codereview.chromium.org/2256773004/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): https://codereview.chromium.org/2256773004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:45: CSV_ROW_HEADERS = 'crbug link, test file, days since last updated, status\n' This would also make sense as a list of strings: CSV_ROW_HEADERS = ['crbug link', 'test file', 'days since last updated', 'status'] https://codereview.chromium.org/2256773004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:77: self.host.filesystem.write_text_file(self.csv_filename, contents) Note: this function could also be implemented with the csv module (https://pymotw.com/2/csv/). It would look something like this: def write_csv(header, rows): out = StringIO.StringIO() writer = csv.writer(out) writer.writerow(headers) for row in rows: writer.writerow(row) self.host.filesystem.write_text_file(self.csv_filename, out.getvalue()) The advantage of this is that the csv module would handle any comma or quote characters in fields (e.g. if you included multiple comma-separated bug links in one field, then that shouldn't be a problem), and it makes some parts of the code simpler. Anyway, if this change is made, it could be made in a separate CL. https://codereview.chromium.org/2256773004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:117: self.state[bug_link] = parsed['state'] It seems like storing the state of the bug isn't closely related to the main purpose of this function (which is called is_stale) -- this is a signal that refactoring would make the code clearer. Currently, a lot of processing and storing things about bugs is done in is_stale... maybe what we actually want to do is to process all of the bugs first and store a list of dicts or objects with information about each bug, *then* go through that list. Would that make sense?
The CQ bit was checked by nainar@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...
Hi Quinten, I made the refactor suggestions you made in this patch. PTAL? and let me know if it still looks good. Thanks! https://codereview.chromium.org/2256773004/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): https://codereview.chromium.org/2256773004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:45: CSV_ROW_HEADERS = 'crbug link, test file, days since last updated, status\n' Done. https://codereview.chromium.org/2256773004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:77: self.host.filesystem.write_text_file(self.csv_filename, contents) Done. https://codereview.chromium.org/2256773004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:117: self.state[bug_link] = parsed['state'] Have done the refactors you have asked for.
Description was changed from ========== Add more informtion to the CSV generated Add CSV row headers, days since last comment and whether or not bug is closed to the CSV BUG=597797 ========== to ========== Add more informtion to the CSV generated Add CSV row headers, days since last comment and whether or not bug is closed and owner to the CSV BUG=597797 ==========
The CQ bit was checked by nainar@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.
I thought I replied last week but it looks like I forgot! Generally looks good; thanks for making those changes. I have a few more nits/suggestions for this CL. https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:49: class Bug_Info(): For consistency, this should be `class BugInfo(object):` - Classes are generally capitalized camel case. - https://wiki.python.org/moin/NewClassVsClassicClass https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:53: self.filename = filename Duplicate line https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:103: # return the stale bug's information Comments should generally start with a capital letter and end with a period. https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:106: return [bug_links[0], self.bug_info[bug_links[0]].filename, self.bug_info[bug_links[0]].days_since_last_update, self.bug_info[bug_links[0]].owner, self.bug_info[bug_links[0]].status] This line could be broken up to improve readability. https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:119: return; Unnecessary semicolon
Hi, Made all the changes you asked for. Not sure what the protocol to submit if I already have an L*TM is. So will wait for you to take a look :) https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:49: class Bug_Info(): Done. https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:53: self.filename = filename Done. https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:103: # return the stale bug's information Done. https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:106: return [bug_links[0], self.bug_info[bug_links[0]].filename, self.bug_info[bug_links[0]].days_since_last_update, self.bug_info[bug_links[0]].owner, self.bug_info[bug_links[0]].status] Done. https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:119: return; Done.
On 2016/08/23 at 00:21:24, nainar wrote: > Hi, > > Made all the changes you asked for. > > Not sure what the protocol to submit if I already have an L*TM is. So will wait for you to take a look :) > > https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries (right): > > https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:49: class Bug_Info(): > Done. > > https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:53: self.filename = filename > Done. > > https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:103: # return the stale bug's information > Done. > > https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:106: return [bug_links[0], self.bug_info[bug_links[0]].filename, self.bug_info[bug_links[0]].days_since_last_update, self.bug_info[bug_links[0]].owner, self.bug_info[bug_links[0]].status] > Done. > > https://codereview.chromium.org/2256773004/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/print-stale-test-expectations-entries:119: return; > Done. If everything is done then I think it's good to submit now, but remember to upload the new patchset first and remove test.csv from this CL :-)
Good point - looks like I got ahead of myself and posted the comments without hitting enter on git cl upload :(
On 2016/08/23 at 02:56:22, nainar wrote: > Good point - looks like I got ahead of myself and posted the comments without hitting enter on git cl upload :( Alright, LGTM :-)
The CQ bit was checked by nainar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add more informtion to the CSV generated Add CSV row headers, days since last comment and whether or not bug is closed and owner to the CSV BUG=597797 ========== to ========== Add more informtion to the CSV generated Add CSV row headers, days since last comment and whether or not bug is closed and owner to the CSV BUG=597797 Committed: https://crrev.com/c3e861ceca16f92cf5eb0f889d208175bc971d3b Cr-Commit-Position: refs/heads/master@{#413664} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c3e861ceca16f92cf5eb0f889d208175bc971d3b Cr-Commit-Position: refs/heads/master@{#413664} |
