mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 43552: Added a support/push-reviews.py script to push reviews upstream.
Date Tue, 23 Feb 2016 00:44:18 GMT


> 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
> 
> Vinod Kone wrote:
>     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.

Looks great.


> 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.
> 
> Vinod Kone wrote:
>     I'm trying to be close to our C++ style. This is how it is in apply-reviews.py too.

>     
>     What did bmahler suggest?

He had suggested putting all parameters on the next line, indedented by 4 spaces instead of
directly after the pranthesis. e.g.

```
parser.add_argument(
    '-n',
    '--dry-run',
    action='store_true',
    help='Perform a dry run.')
```


> 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?
> 
> Vinod Kone wrote:
>     "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.

Yeah, this is simpler because otherwise you could run into a situation where tracking branch
was something like "klueska/my-branch" and remote was set to "origin", causing a conflict
that we would need to check for.  It's hard for me to imagine a situation where you would
be wanting to run this script anywhere other than master, so this change makes sense.


> 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/
> 
> Vinod Kone wrote:
>     I'll make a TODO for now as committers don't currently/always set the commit message
on RB.

I think we should add the option as a command line flag now, so that people who like to include
the commit message when closing can still take advantage of this script.  If we later decide
that *everyone* should always include the commit message, we can remove the flag and force
it to be set.


- Kevin


-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message