> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > In general, I prefer scripts with a bunch of helper functions and a compact 
> > main() that steps through each of them. I'm not sure what the general 
> > concensus for the Mesos code base is, but I generally find this easier to 
> > walk through.  Example: support/generate-endpoint-help.py

In our C++ code base we generally prefer procedural style as it requires fewer 
context switches. We haven't officially defined the style for Python code in 
our code base, but I would guess we want to be similar. 

That said if something makes sense to be a function (e.g, reuse) we should 
extract it into a function. I've added a main and refactored a bit. Let me know 
if it looks ok.


> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > support/push-reviews.py, lines 27-30
> > <https://reviews.apache.org/r/43552/diff/1/?file=1252228#file1252228line27>
> >
> >     Do we have guidelines for indentation here?  When writing the 
> > support/generate-endpoints-help.py script, bmahler suggested something 
> > different than what you have here.

I'm trying to be close to our C++ style. This is how it is in apply-reviews.py 
too. 

What did bmahler suggest?


> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > support/push-reviews.py, lines 32-39
> > <https://reviews.apache.org/r/43552/diff/1/?file=1252228#file1252228line32>
> >
> >     If the tracking branch is reauired to be of the form `remote/branch` 
> > (as mentioned in a later comment), what do we need the `remote` flag for?

"tracking branch" is mainly required to figure out the new commits to push. 
Technically it can be just "master". "remote" is to figure out the upstream to 
push to. If remote is not specified, we try to deduce it from "tracking 
branch", in which case we expect it to be a "remote tracking branch". 

But I agree it's all confusing. I will kill both "--remote" and 
"--tracking_branch" and expect users to run this from "master" branch for 
simplicity.


> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > support/push-reviews.py, line 52
> > <https://reviews.apache.org/r/43552/diff/1/?file=1252228#file1252228line52>
> >
> >     We need some sort of way to enforce that the tracking branch is always 
> > of the form `remote/branch`. Otherwise, if we run e.g.
> >     
> >     ```
> >     $ klueska@c99:~/projects/mesos$ support/push-reviews.py -t master
> >     ```
> >     
> >     remote gets set to `master` here, which obviously breaks things later 
> > on.

see above.


> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > support/push-reviews.py, lines 108-112
> > <https://reviews.apache.org/r/43552/diff/1/?file=1252228#file1252228line108>
> >
> >     As we close the review, we should also post a comment to reviewboard 
> > with the commit message we actually pushed to master.
> >     
> >     You can use the --description flag:
> >     
> >     https://www.reviewboard.org/docs/rbtools/dev/rbt/commands/close/

I'll make a TODO for now as committers don't currently/always set the commit 
message on RB.


- Vinod


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43552/#review119507
-----------------------------------------------------------


On Feb. 22, 2016, 4:52 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43552/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2016, 4:52 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Kevin Klues, and Michael Park.
> 
> 
> Bugs: MESOS-3929
>     https://issues.apache.org/jira/browse/MESOS-3929
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This script allows committers to push locally applied review chain to the ASF
> git repo and mark the reviews as submitted.
> 
> 
> Diffs
> -----
> 
>   support/push-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43552/diff/
> 
> 
> Testing
> -------
> 
> Tested locally.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Reply via email to