----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67503/#review204669 -----------------------------------------------------------
I would rename the script `get-dependent-reviews.py` to be more explicit. Regarding the commit: its message should be more descriptive regarding the reasons behind this new support script and the Testing Done part should be filled with at least some tests or the fact that tests can be read later in the chain. support/python3/get-review-ids.py Lines 18 (patched) <https://reviews.apache.org/r/67503/#comment287272> Missing docstring. support/python3/get-review-ids.py Lines 21 (patched) <https://reviews.apache.org/r/67503/#comment287264> `# noqa`? Also, we generally make each import its own line (at least in the Python CLI codebase). support/python3/get-review-ids.py Lines 25 (patched) <https://reviews.apache.org/r/67503/#comment287273> Missing docstring. support/python3/get-review-ids.py Lines 29 (patched) <https://reviews.apache.org/r/67503/#comment287265> Why is it required? Not having an output file should result in having the dependent review IDs printed which is done in the last for loop of `support/python3/common.py`. support/python3/get-review-ids.py Lines 35 (patched) <https://reviews.apache.org/r/67503/#comment287274> Missing docstring. - Armand Grillet On June 8, 2018, 12:07 p.m., Dragos Schebesch wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67503/ > ----------------------------------------------------------- > > (Updated June 8, 2018, 12:07 p.m.) > > > Review request for mesos and Andrew Schwartzmeyer. > > > Repository: mesos > > > Description > ------- > > Added helper for fetching review id > > > Diffs > ----- > > support/python3/get-review-ids.py PRE-CREATION > > > Diff: https://reviews.apache.org/r/67503/diff/1/ > > > Testing > ------- > > > Thanks, > > Dragos Schebesch > >
