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