----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67502/#review204667 -----------------------------------------------------------
Regarding the commit: its message should be more descriptive regarding the reasons behind this new file 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/common.py Lines 1 (patched) <https://reviews.apache.org/r/67502/#comment287258> `#!/usr/bin/env python3`? support/python3/common.py Lines 18 (patched) <https://reviews.apache.org/r/67502/#comment287259> Missing docstring. Could be: ``` # limitations under the License. """ These common helper classes help to manage the connection between the machine and ReviewBoard. """ from datetime import datetime ``` support/python3/common.py Lines 34 (patched) <https://reviews.apache.org/r/67502/#comment287260> Missing class docstring. support/python3/common.py Lines 40 (patched) <https://reviews.apache.org/r/67502/#comment287261> Having [] as argument is dangerous. The problem with a mutable default argument is that it will be shared between all invocations of the function. This should be replaced by: ``` _review_ids(self, review_request, review_ids=None): if review_ids is None: review_ids = [] ``` support/python3/common.py Lines 58 (patched) <https://reviews.apache.org/r/67502/#comment287266> s/`Call`/`Calls` support/python3/common.py Lines 79 (patched) <https://reviews.apache.org/r/67502/#comment287269> s/`get_review_ids`/`get_dependent_review_ids` support/python3/common.py Lines 90 (patched) <https://reviews.apache.org/r/67502/#comment287267> s/`Post`/`Posts` support/python3/common.py Lines 107 (patched) <https://reviews.apache.org/r/67502/#comment287268> s/`Post`/`Posts` support/python3/common.py Lines 108 (patched) <https://reviews.apache.org/r/67502/#comment287270> s.`review:`/`review` support/python3/common.py Lines 114 (patched) <https://reviews.apache.org/r/67502/#comment287263> r/`already already`/`already` support/python3/common.py Lines 147 (patched) <https://reviews.apache.org/r/67502/#comment287271> This could be more descriptive, e.g. `This patch has been updated since its last review, needs verification`. Also, sometimes devs rebase the review request but the diff is empty and in that case the diff should not need a new verification. - 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/67502/ > ----------------------------------------------------------- > > (Updated June 8, 2018, 12:07 p.m.) > > > Review request for mesos and Andrew Schwartzmeyer. > > > Repository: mesos > > > Description > ------- > > Refactored API functionality into separate module > > > Diffs > ----- > > support/python3/common.py PRE-CREATION > > > Diff: https://reviews.apache.org/r/67502/diff/1/ > > > Testing > ------- > > > Thanks, > > Dragos Schebesch > >
