Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6722014:
For now, stop suggesting reviewers during 'gcl change'. (Closed)
|
Created:
9 years, 9 months ago by Dirk Pranke Modified:
9 years, 7 months ago CC:
chromium-reviews, M-A Ruel Visibility:
Public. |
DescriptionFor now, stop suggesting reviewers during 'gcl change'.
This fixes an issue where we would suggest '*' and then choke on the text
down the road. Also, the suggestions were not yet very useful.
R=maruel@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79227
Patch Set 1 #Messages
Total messages: 16 (0 generated)
when you readd this, please don't add it to the bottom of cl descriptions. can it be automatically sent to rietveld out-of-band, so that the cl description doesn't include this? also, it would be good to not keep readding this if i remove it. also, do you know who added the "TEST=none; no visible change" stuff? On Wed, Mar 23, 2011 at 2:21 PM, <dpranke@chromium.org> wrote: > Reviewers: Marc-Antoine Ruel, > > Description: > For now, stop suggesting reviewers during 'gcl change'. > > This fixes an issue where we would suggest '*' and then choke on the text > down the road. Also, the suggestions were not yet very useful. > > R=maruel@chromium.org > > > Please review this at http://codereview.chromium.org/6722014/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/tools/depot_tools > > Affected files: > M gcl.py > M tests/gcl_unittest.py > > > Index: gcl.py > diff --git a/gcl.py b/gcl.py > index > 20762d1be0c5bdbb277170c046608585f459397a..fe04b99af8514bf90f5130cdf15ddb4034f6c16e > 100755 > --- a/gcl.py > +++ b/gcl.py > @@ -763,12 +763,6 @@ def OptionallyDoPresubmitChecks(change_info, > committing, args): > return DoPresubmitChecks(change_info, committing, True) > > > -def suggest_reviewers(change_info, affected_files): > - owners_db = owners.Database(change_info.GetLocalRoot(), fopen=file, > - os_path=os.path) > - return owners_db.reviewers_for([f[1] for f in affected_files]) > - > - > def defer_attributes(a, b): > """Copy attributes from an object (like a function) to another.""" > for x in dir(a): > @@ -1098,15 +1092,6 @@ def CMDchange(args): > affected_files = [x for x in other_files if file_re.match(x[0])] > unaffected_files = [x for x in other_files if not file_re.match(x[0])] > > - if not change_info.reviewers: > - files_for_review = affected_files[:] > - files_for_review.extend(change_info.GetFiles()) > - suggested_reviewers = suggest_reviewers(change_info, files_for_review) > - if suggested_reviewers: > - reviewers_re = re.compile(REVIEWERS_REGEX) > - if not any(reviewers_re.match(l) for l in description.splitlines()): > - description += '\n\nR=' + ','.join(suggested_reviewers) > - > description = description.rstrip() + '\n' > > separator1 = ("\n---All lines above this line become the description.\n" > Index: tests/gcl_unittest.py > diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py > index > 4ab2b6fc17e91463ab511115bd1a46056364b3cb..26cc2b17434808bd41f4aaed2249b1228f065643 > 100755 > --- a/tests/gcl_unittest.py > +++ b/tests/gcl_unittest.py > @@ -93,8 +93,8 @@ class GclUnittest(GclTestsBase): > 'attrs', 'breakpad', 'defer_attributes', 'gclient_utils', > 'getpass', > 'json', 'main', 'need_change', 'need_change_and_args', 'no_args', > 'optparse', 'os', 'owners', 'presubmit_support', 'random', 're', > - 'string', 'subprocess', 'suggest_reviewers', 'sys', 'tempfile', > - 'time', 'upload', 'urllib2', > + 'string', 'subprocess', 'sys', 'tempfile', 'time', 'upload', > + 'urllib2', > ] > # If this test fails, you should add the relevant test. > self.compareMembers(gcl, members) > > >
On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek <jam@chromium.org> wrote: > when you readd this, please don't add it to the bottom of cl descriptions. > can it be automatically sent to rietveld out-of-band, so that the cl > description doesn't include this? also, it would be good to not keep > readding this if i remove it. > > also, do you know who added the "TEST=none; no visible change" stuff? > argh nm, i thought that was my cl and it wasn't > > > On Wed, Mar 23, 2011 at 2:21 PM, <dpranke@chromium.org> wrote: > >> Reviewers: Marc-Antoine Ruel, >> >> Description: >> For now, stop suggesting reviewers during 'gcl change'. >> >> This fixes an issue where we would suggest '*' and then choke on the text >> down the road. Also, the suggestions were not yet very useful. >> >> R=maruel@chromium.org >> >> >> Please review this at http://codereview.chromium.org/6722014/ >> >> SVN Base: svn://svn.chromium.org/chrome/trunk/tools/depot_tools >> >> Affected files: >> M gcl.py >> M tests/gcl_unittest.py >> >> >> Index: gcl.py >> diff --git a/gcl.py b/gcl.py >> index >> 20762d1be0c5bdbb277170c046608585f459397a..fe04b99af8514bf90f5130cdf15ddb4034f6c16e >> 100755 >> --- a/gcl.py >> +++ b/gcl.py >> @@ -763,12 +763,6 @@ def OptionallyDoPresubmitChecks(change_info, >> committing, args): >> return DoPresubmitChecks(change_info, committing, True) >> >> >> -def suggest_reviewers(change_info, affected_files): >> - owners_db = owners.Database(change_info.GetLocalRoot(), fopen=file, >> - os_path=os.path) >> - return owners_db.reviewers_for([f[1] for f in affected_files]) >> - >> - >> def defer_attributes(a, b): >> """Copy attributes from an object (like a function) to another.""" >> for x in dir(a): >> @@ -1098,15 +1092,6 @@ def CMDchange(args): >> affected_files = [x for x in other_files if file_re.match(x[0])] >> unaffected_files = [x for x in other_files if not file_re.match(x[0])] >> >> - if not change_info.reviewers: >> - files_for_review = affected_files[:] >> - files_for_review.extend(change_info.GetFiles()) >> - suggested_reviewers = suggest_reviewers(change_info, >> files_for_review) >> - if suggested_reviewers: >> - reviewers_re = re.compile(REVIEWERS_REGEX) >> - if not any(reviewers_re.match(l) for l in >> description.splitlines()): >> - description += '\n\nR=' + ','.join(suggested_reviewers) >> - >> description = description.rstrip() + '\n' >> >> separator1 = ("\n---All lines above this line become the description.\n" >> Index: tests/gcl_unittest.py >> diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py >> index >> 4ab2b6fc17e91463ab511115bd1a46056364b3cb..26cc2b17434808bd41f4aaed2249b1228f065643 >> 100755 >> --- a/tests/gcl_unittest.py >> +++ b/tests/gcl_unittest.py >> @@ -93,8 +93,8 @@ class GclUnittest(GclTestsBase): >> 'attrs', 'breakpad', 'defer_attributes', 'gclient_utils', >> 'getpass', >> 'json', 'main', 'need_change', 'need_change_and_args', 'no_args', >> 'optparse', 'os', 'owners', 'presubmit_support', 'random', 're', >> - 'string', 'subprocess', 'suggest_reviewers', 'sys', 'tempfile', >> - 'time', 'upload', 'urllib2', >> + 'string', 'subprocess', 'sys', 'tempfile', 'time', 'upload', >> + 'urllib2', >> ] >> # If this test fails, you should add the relevant test. >> self.compareMembers(gcl, members) >> >> >> >
On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek <jam@chromium.org> wrote: > when you readd this, please don't add it to the bottom of cl descriptions. > can it be automatically sent to rietveld out-of-band, so that the cl > description doesn't include this? also, it would be good to not keep > readding this if i remove it. It is my intent so that the R= shows up in your $EDITOR but is stripped from the description on sending to Rietveld (and the reviewers get set correctly in Rietveld instead). I think it doesn't quite work correctly at the moment but I have another patch pending that should fix it. I'm not sure if that's quite what you're asking for. My thinking is that I always want the reviewers to be displayed when I have the change open in the editor so I can see who it will be set to and change it inline as necessary. Are you looking for something else? -- Dirk
On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke <dpranke@chromium.org> wrote: > On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek <jam@chromium.org> > wrote: > > when you readd this, please don't add it to the bottom of cl > descriptions. > > can it be automatically sent to rietveld out-of-band, so that the cl > > description doesn't include this? also, it would be good to not keep > > readding this if i remove it. > > It is my intent so that the R= shows up in your $EDITOR but is > stripped from the description on sending to Rietveld (and the > reviewers get set correctly in Rietveld instead). > ah ok, that sounds perfect > > I think it doesn't quite work correctly at the moment but I have > another patch pending that should fix it. > > I'm not sure if that's quite what you're asking for. My thinking is > that I always want the reviewers to be displayed when I have the > change open in the editor so I can see who it will be set to and > change it inline as necessary. > Are you looking for something else? > right now when i touch a bunch of files, i get a huge list of reviewers. if i edit it, i dont want to have that change lost each time i run gcl change to add one file.. > > -- Dirk >
On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke <dpranke@chromium.org> wrote: >> >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek <jam@chromium.org> >> wrote: >> > when you readd this, please don't add it to the bottom of cl >> > descriptions. >> > can it be automatically sent to rietveld out-of-band, so that the cl >> > description doesn't include this? also, it would be good to not keep >> > readding this if i remove it. >> >> It is my intent so that the R= shows up in your $EDITOR but is >> stripped from the description on sending to Rietveld (and the >> reviewers get set correctly in Rietveld instead). > > ah ok, that sounds perfect > >> >> I think it doesn't quite work correctly at the moment but I have >> another patch pending that should fix it. >> >> I'm not sure if that's quite what you're asking for. My thinking is >> that I always want the reviewers to be displayed when I have the >> change open in the editor so I can see who it will be set to and >> change it inline as necessary. >> >> Are you looking for something else? > > right now when i touch a bunch of files, i get a huge list of reviewers. if > i edit it, i dont want to have that change lost each time i run gcl change > to add one file.. Right, it definitely shouldn't be doing that. Perhaps at some point it'll note when your existing list of reviewers doesn't cover all of your files, but for now (at least, as of when my patch lands) I think it'll only suggest reviewers if there aren't any at all. -- Dirk
On Wed, Mar 23, 2011 at 3:36 PM, Dirk Pranke <dpranke@chromium.org> wrote: > On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek <jam@chromium.org> > wrote: > > > > > > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke <dpranke@chromium.org> > wrote: > >> > >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek <jam@chromium.org> > >> wrote: > >> > when you readd this, please don't add it to the bottom of cl > >> > descriptions. > >> > can it be automatically sent to rietveld out-of-band, so that the cl > >> > description doesn't include this? also, it would be good to not keep > >> > readding this if i remove it. > >> > >> It is my intent so that the R= shows up in your $EDITOR but is > >> stripped from the description on sending to Rietveld (and the > >> reviewers get set correctly in Rietveld instead). > > > > ah ok, that sounds perfect > > > >> > >> I think it doesn't quite work correctly at the moment but I have > >> another patch pending that should fix it. > >> > >> I'm not sure if that's quite what you're asking for. My thinking is > >> that I always want the reviewers to be displayed when I have the > >> change open in the editor so I can see who it will be set to and > >> change it inline as necessary. > >> > >> Are you looking for something else? > > > > right now when i touch a bunch of files, i get a huge list of reviewers. > if > > i edit it, i dont want to have that change lost each time i run gcl > change > > to add one file.. > > Right, it definitely shouldn't be doing that. Perhaps at some point > it'll note when > your existing list of reviewers doesn't cover all of your files, but > for now (at least, > as of when my patch lands) I think it'll only suggest reviewers if there > aren't > any at all. > does it pull it from rietveld to stay in sync, just like what happens with the description? my use case is that i might run gcl change a number of times before finally uploading. if i remove that big list of reviewers, i would prefer that it doesnt keep getting added. > > -- Dirk >
On Wed, Mar 23, 2011 at 3:44 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Wed, Mar 23, 2011 at 3:36 PM, Dirk Pranke <dpranke@chromium.org> wrote: > >> On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek <jam@chromium.org> >> wrote: >> > >> > >> > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke <dpranke@chromium.org> >> wrote: >> >> >> >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek <jam@chromium.org> >> >> wrote: >> >> > when you readd this, please don't add it to the bottom of cl >> >> > descriptions. >> >> > can it be automatically sent to rietveld out-of-band, so that the cl >> >> > description doesn't include this? also, it would be good to not keep >> >> > readding this if i remove it. >> >> >> >> It is my intent so that the R= shows up in your $EDITOR but is >> >> stripped from the description on sending to Rietveld (and the >> >> reviewers get set correctly in Rietveld instead). >> > >> > ah ok, that sounds perfect >> > >> >> >> >> I think it doesn't quite work correctly at the moment but I have >> >> another patch pending that should fix it. >> >> >> >> I'm not sure if that's quite what you're asking for. My thinking is >> >> that I always want the reviewers to be displayed when I have the >> >> change open in the editor so I can see who it will be set to and >> >> change it inline as necessary. >> >> >> >> Are you looking for something else? >> > >> > right now when i touch a bunch of files, i get a huge list of reviewers. >> if >> > i edit it, i dont want to have that change lost each time i run gcl >> change >> > to add one file.. >> >> Right, it definitely shouldn't be doing that. Perhaps at some point >> it'll note when >> your existing list of reviewers doesn't cover all of your files, but >> for now (at least, >> as of when my patch lands) I think it'll only suggest reviewers if there >> aren't >> any at all. >> > > does it pull it from rietveld to stay in sync, just like what happens with > the description? > > my use case is that i might run gcl change a number of times before finally > uploading. if i remove that big list of reviewers, i would prefer that it > doesnt keep getting added. > to clarify, i personally don't like to put R= in the desc > >> -- Dirk >> > >
On Wed, Mar 23, 2011 at 3:44 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Wed, Mar 23, 2011 at 3:36 PM, Dirk Pranke <dpranke@chromium.org> wrote: >> >> On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek <jam@chromium.org> >> wrote: >> > >> > >> > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke <dpranke@chromium.org> >> > wrote: >> >> >> >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek <jam@chromium.org> >> >> wrote: >> >> > when you readd this, please don't add it to the bottom of cl >> >> > descriptions. >> >> > can it be automatically sent to rietveld out-of-band, so that the cl >> >> > description doesn't include this? also, it would be good to not keep >> >> > readding this if i remove it. >> >> >> >> It is my intent so that the R= shows up in your $EDITOR but is >> >> stripped from the description on sending to Rietveld (and the >> >> reviewers get set correctly in Rietveld instead). >> > >> > ah ok, that sounds perfect >> > >> >> >> >> I think it doesn't quite work correctly at the moment but I have >> >> another patch pending that should fix it. >> >> >> >> I'm not sure if that's quite what you're asking for. My thinking is >> >> that I always want the reviewers to be displayed when I have the >> >> change open in the editor so I can see who it will be set to and >> >> change it inline as necessary. >> >> >> >> Are you looking for something else? >> > >> > right now when i touch a bunch of files, i get a huge list of reviewers. >> > if >> > i edit it, i dont want to have that change lost each time i run gcl >> > change >> > to add one file.. >> >> Right, it definitely shouldn't be doing that. Perhaps at some point >> it'll note when >> your existing list of reviewers doesn't cover all of your files, but >> for now (at least, >> as of when my patch lands) I think it'll only suggest reviewers if there >> aren't >> any at all. > > does it pull it from rietveld to stay in sync, just like what happens with > the description? Yes. > my use case is that i might run gcl change a number of times before finally > uploading. if i remove that big list of reviewers, i would prefer that it > doesnt keep getting added. If you run change after the initial upload, it should insert "R=" plus the actual list of reviewers from the change. It should only ever suggest reviewers if there aren't any in the change. -- Dirk
lgtm
On Wed, Mar 23, 2011 at 3:52 PM, Dirk Pranke <dpranke@chromium.org> wrote: > On Wed, Mar 23, 2011 at 3:44 PM, John Abd-El-Malek <jam@chromium.org> > wrote: > > > > > > On Wed, Mar 23, 2011 at 3:36 PM, Dirk Pranke <dpranke@chromium.org> > wrote: > >> > >> On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek <jam@chromium.org> > >> wrote: > >> > > >> > > >> > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke <dpranke@chromium.org> > >> > wrote: > >> >> > >> >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek <jam@chromium.org > > > >> >> wrote: > >> >> > when you readd this, please don't add it to the bottom of cl > >> >> > descriptions. > >> >> > can it be automatically sent to rietveld out-of-band, so that the > cl > >> >> > description doesn't include this? also, it would be good to not > keep > >> >> > readding this if i remove it. > >> >> > >> >> It is my intent so that the R= shows up in your $EDITOR but is > >> >> stripped from the description on sending to Rietveld (and the > >> >> reviewers get set correctly in Rietveld instead). > >> > > >> > ah ok, that sounds perfect > >> > > >> >> > >> >> I think it doesn't quite work correctly at the moment but I have > >> >> another patch pending that should fix it. > >> >> > >> >> I'm not sure if that's quite what you're asking for. My thinking is > >> >> that I always want the reviewers to be displayed when I have the > >> >> change open in the editor so I can see who it will be set to and > >> >> change it inline as necessary. > >> >> > >> >> Are you looking for something else? > >> > > >> > right now when i touch a bunch of files, i get a huge list of > reviewers. > >> > if > >> > i edit it, i dont want to have that change lost each time i run gcl > >> > change > >> > to add one file.. > >> > >> Right, it definitely shouldn't be doing that. Perhaps at some point > >> it'll note when > >> your existing list of reviewers doesn't cover all of your files, but > >> for now (at least, > >> as of when my patch lands) I think it'll only suggest reviewers if there > >> aren't > >> any at all. > > > > does it pull it from rietveld to stay in sync, just like what happens > with > > the description? > > Yes. > > > my use case is that i might run gcl change a number of times before > finally > > uploading. if i remove that big list of reviewers, i would prefer that > it > > doesnt keep getting added. > > If you run change after the initial upload, it should insert "R=" plus > the actual > list of reviewers from the change. It should only ever suggest > reviewers if there > aren't any in the change. > that's what I don't like though. i.e. I run gcl change to create the list initially, and remove the list of reviewers. then each time i run gcl change it adds them. can this only be done on change creation? > > -- Dirk >
On Wed, Mar 23, 2011 at 4:57 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Wed, Mar 23, 2011 at 3:52 PM, Dirk Pranke <dpranke@chromium.org> wrote: >> >> On Wed, Mar 23, 2011 at 3:44 PM, John Abd-El-Malek <jam@chromium.org> >> wrote: >> > >> > >> > On Wed, Mar 23, 2011 at 3:36 PM, Dirk Pranke <dpranke@chromium.org> >> > wrote: >> >> >> >> On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek <jam@chromium.org> >> >> wrote: >> >> > >> >> > >> >> > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke <dpranke@chromium.org> >> >> > wrote: >> >> >> >> >> >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek >> >> >> <jam@chromium.org> >> >> >> wrote: >> >> >> > when you readd this, please don't add it to the bottom of cl >> >> >> > descriptions. >> >> >> > can it be automatically sent to rietveld out-of-band, so that the >> >> >> > cl >> >> >> > description doesn't include this? also, it would be good to not >> >> >> > keep >> >> >> > readding this if i remove it. >> >> >> >> >> >> It is my intent so that the R= shows up in your $EDITOR but is >> >> >> stripped from the description on sending to Rietveld (and the >> >> >> reviewers get set correctly in Rietveld instead). >> >> > >> >> > ah ok, that sounds perfect >> >> > >> >> >> >> >> >> I think it doesn't quite work correctly at the moment but I have >> >> >> another patch pending that should fix it. >> >> >> >> >> >> I'm not sure if that's quite what you're asking for. My thinking is >> >> >> that I always want the reviewers to be displayed when I have the >> >> >> change open in the editor so I can see who it will be set to and >> >> >> change it inline as necessary. >> >> >> >> >> >> Are you looking for something else? >> >> > >> >> > right now when i touch a bunch of files, i get a huge list of >> >> > reviewers. >> >> > if >> >> > i edit it, i dont want to have that change lost each time i run gcl >> >> > change >> >> > to add one file.. >> >> >> >> Right, it definitely shouldn't be doing that. Perhaps at some point >> >> it'll note when >> >> your existing list of reviewers doesn't cover all of your files, but >> >> for now (at least, >> >> as of when my patch lands) I think it'll only suggest reviewers if >> >> there >> >> aren't >> >> any at all. >> > >> > does it pull it from rietveld to stay in sync, just like what happens >> > with >> > the description? >> >> Yes. >> >> > my use case is that i might run gcl change a number of times before >> > finally >> > uploading. if i remove that big list of reviewers, i would prefer that >> > it >> > doesnt keep getting added. >> >> If you run change after the initial upload, it should insert "R=" plus >> the actual >> list of reviewers from the change. It should only ever suggest >> reviewers if there >> aren't any in the change. > > that's what I don't like though. i.e. I run gcl change to create the list > initially, and remove the list of reviewers. then each time i run gcl > change it adds them. can this only be done on change creation? You're talking about running 'gcl change' multiple times prior to 'gcl upload'? Yeah, we could make the change such that we only suggested people on the initial creation of the issue. I could probably also add a --don't-suggest-any-reviewers sort of flag or config setting if you would prefer to just stop this nonsense once and for all. -- Dirk
no flag please Le 23 mars 2011 20:22, "Dirk Pranke" <dpranke@chromium.org> a écrit : > On Wed, Mar 23, 2011 at 4:57 PM, John Abd-El-Malek <jam@chromium.org> wrote: >> >> >> On Wed, Mar 23, 2011 at 3:52 PM, Dirk Pranke <dpranke@chromium.org> wrote: >>> >>> On Wed, Mar 23, 2011 at 3:44 PM, John Abd-El-Malek <jam@chromium.org> >>> wrote: >>> > >>> > >>> > On Wed, Mar 23, 2011 at 3:36 PM, Dirk Pranke <dpranke@chromium.org> >>> > wrote: >>> >> >>> >> On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek <jam@chromium.org> >>> >> wrote: >>> >> > >>> >> > >>> >> > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke <dpranke@chromium.org> >>> >> > wrote: >>> >> >> >>> >> >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek >>> >> >> <jam@chromium.org> >>> >> >> wrote: >>> >> >> > when you readd this, please don't add it to the bottom of cl >>> >> >> > descriptions. >>> >> >> > can it be automatically sent to rietveld out-of-band, so that the >>> >> >> > cl >>> >> >> > description doesn't include this? also, it would be good to not >>> >> >> > keep >>> >> >> > readding this if i remove it. >>> >> >> >>> >> >> It is my intent so that the R= shows up in your $EDITOR but is >>> >> >> stripped from the description on sending to Rietveld (and the >>> >> >> reviewers get set correctly in Rietveld instead). >>> >> > >>> >> > ah ok, that sounds perfect >>> >> > >>> >> >> >>> >> >> I think it doesn't quite work correctly at the moment but I have >>> >> >> another patch pending that should fix it. >>> >> >> >>> >> >> I'm not sure if that's quite what you're asking for. My thinking is >>> >> >> that I always want the reviewers to be displayed when I have the >>> >> >> change open in the editor so I can see who it will be set to and >>> >> >> change it inline as necessary. >>> >> >> >>> >> >> Are you looking for something else? >>> >> > >>> >> > right now when i touch a bunch of files, i get a huge list of >>> >> > reviewers. >>> >> > if >>> >> > i edit it, i dont want to have that change lost each time i run gcl >>> >> > change >>> >> > to add one file.. >>> >> >>> >> Right, it definitely shouldn't be doing that. Perhaps at some point >>> >> it'll note when >>> >> your existing list of reviewers doesn't cover all of your files, but >>> >> for now (at least, >>> >> as of when my patch lands) I think it'll only suggest reviewers if >>> >> there >>> >> aren't >>> >> any at all. >>> > >>> > does it pull it from rietveld to stay in sync, just like what happens >>> > with >>> > the description? >>> >>> Yes. >>> >>> > my use case is that i might run gcl change a number of times before >>> > finally >>> > uploading. if i remove that big list of reviewers, i would prefer that >>> > it >>> > doesnt keep getting added. >>> >>> If you run change after the initial upload, it should insert "R=" plus >>> the actual >>> list of reviewers from the change. It should only ever suggest >>> reviewers if there >>> aren't any in the change. >> >> that's what I don't like though. i.e. I run gcl change to create the list >> initially, and remove the list of reviewers. then each time i run gcl >> change it adds them. can this only be done on change creation? > > You're talking about running 'gcl change' multiple times prior to 'gcl upload'? > > Yeah, we could make the change such that we only suggested people on > the initial creation of the issue. > > I could probably also add a --don't-suggest-any-reviewers sort of flag > or config setting if you would prefer to just stop this > nonsense once and for all. > > -- Dirk
On Wed, Mar 23, 2011 at 5:22 PM, Dirk Pranke <dpranke@chromium.org> wrote: > On Wed, Mar 23, 2011 at 4:57 PM, John Abd-El-Malek <jam@chromium.org> > wrote: > > > > > > On Wed, Mar 23, 2011 at 3:52 PM, Dirk Pranke <dpranke@chromium.org> > wrote: > >> > >> On Wed, Mar 23, 2011 at 3:44 PM, John Abd-El-Malek <jam@chromium.org> > >> wrote: > >> > > >> > > >> > On Wed, Mar 23, 2011 at 3:36 PM, Dirk Pranke <dpranke@chromium.org> > >> > wrote: > >> >> > >> >> On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek <jam@chromium.org > > > >> >> wrote: > >> >> > > >> >> > > >> >> > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke <dpranke@chromium.org > > > >> >> > wrote: > >> >> >> > >> >> >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek > >> >> >> <jam@chromium.org> > >> >> >> wrote: > >> >> >> > when you readd this, please don't add it to the bottom of cl > >> >> >> > descriptions. > >> >> >> > can it be automatically sent to rietveld out-of-band, so that > the > >> >> >> > cl > >> >> >> > description doesn't include this? also, it would be good to not > >> >> >> > keep > >> >> >> > readding this if i remove it. > >> >> >> > >> >> >> It is my intent so that the R= shows up in your $EDITOR but is > >> >> >> stripped from the description on sending to Rietveld (and the > >> >> >> reviewers get set correctly in Rietveld instead). > >> >> > > >> >> > ah ok, that sounds perfect > >> >> > > >> >> >> > >> >> >> I think it doesn't quite work correctly at the moment but I have > >> >> >> another patch pending that should fix it. > >> >> >> > >> >> >> I'm not sure if that's quite what you're asking for. My thinking > is > >> >> >> that I always want the reviewers to be displayed when I have the > >> >> >> change open in the editor so I can see who it will be set to and > >> >> >> change it inline as necessary. > >> >> >> > >> >> >> Are you looking for something else? > >> >> > > >> >> > right now when i touch a bunch of files, i get a huge list of > >> >> > reviewers. > >> >> > if > >> >> > i edit it, i dont want to have that change lost each time i run gcl > >> >> > change > >> >> > to add one file.. > >> >> > >> >> Right, it definitely shouldn't be doing that. Perhaps at some point > >> >> it'll note when > >> >> your existing list of reviewers doesn't cover all of your files, but > >> >> for now (at least, > >> >> as of when my patch lands) I think it'll only suggest reviewers if > >> >> there > >> >> aren't > >> >> any at all. > >> > > >> > does it pull it from rietveld to stay in sync, just like what happens > >> > with > >> > the description? > >> > >> Yes. > >> > >> > my use case is that i might run gcl change a number of times before > >> > finally > >> > uploading. if i remove that big list of reviewers, i would prefer > that > >> > it > >> > doesnt keep getting added. > >> > >> If you run change after the initial upload, it should insert "R=" plus > >> the actual > >> list of reviewers from the change. It should only ever suggest > >> reviewers if there > >> aren't any in the change. > > > > that's what I don't like though. i.e. I run gcl change to create the > list > > initially, and remove the list of reviewers. then each time i run gcl > > change it adds them. can this only be done on change creation? > > You're talking about running 'gcl change' multiple times prior to 'gcl > upload'? > yep > > Yeah, we could make the change such that we only suggested people on > the initial creation of the issue. > i think this is good. it's like how we only add files to a cl the first time it's created, and afterwards, the user has to move the modified files above the magic marker. > > I could probably also add a --don't-suggest-any-reviewers sort of flag > or config setting if you would prefer to just stop this > nonsense once and for all. > > -- Dirk >
On Wed, Mar 23, 2011 at 5:23 PM, Marc-Antoine Ruel <maruel@chromium.org>wrote: > no flag please > agreed > Le 23 mars 2011 20:22, "Dirk Pranke" <dpranke@chromium.org> a écrit : > > > On Wed, Mar 23, 2011 at 4:57 PM, John Abd-El-Malek <jam@chromium.org> > wrote: > >> > >> > >> On Wed, Mar 23, 2011 at 3:52 PM, Dirk Pranke <dpranke@chromium.org> > wrote: > >>> > >>> On Wed, Mar 23, 2011 at 3:44 PM, John Abd-El-Malek <jam@chromium.org> > >>> wrote: > >>> > > >>> > > >>> > On Wed, Mar 23, 2011 at 3:36 PM, Dirk Pranke <dpranke@chromium.org> > >>> > wrote: > >>> >> > >>> >> On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek < > jam@chromium.org> > >>> >> wrote: > >>> >> > > >>> >> > > >>> >> > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke < > dpranke@chromium.org> > >>> >> > wrote: > >>> >> >> > >>> >> >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek > >>> >> >> <jam@chromium.org> > >>> >> >> wrote: > >>> >> >> > when you readd this, please don't add it to the bottom of cl > >>> >> >> > descriptions. > >>> >> >> > can it be automatically sent to rietveld out-of-band, so that > the > >>> >> >> > cl > >>> >> >> > description doesn't include this? also, it would be good to > not > >>> >> >> > keep > >>> >> >> > readding this if i remove it. > >>> >> >> > >>> >> >> It is my intent so that the R= shows up in your $EDITOR but is > >>> >> >> stripped from the description on sending to Rietveld (and the > >>> >> >> reviewers get set correctly in Rietveld instead). > >>> >> > > >>> >> > ah ok, that sounds perfect > >>> >> > > >>> >> >> > >>> >> >> I think it doesn't quite work correctly at the moment but I have > >>> >> >> another patch pending that should fix it. > >>> >> >> > >>> >> >> I'm not sure if that's quite what you're asking for. My thinking > is > >>> >> >> that I always want the reviewers to be displayed when I have the > >>> >> >> change open in the editor so I can see who it will be set to and > >>> >> >> change it inline as necessary. > >>> >> >> > >>> >> >> Are you looking for something else? > >>> >> > > >>> >> > right now when i touch a bunch of files, i get a huge list of > >>> >> > reviewers. > >>> >> > if > >>> >> > i edit it, i dont want to have that change lost each time i run > gcl > >>> >> > change > >>> >> > to add one file.. > >>> >> > >>> >> Right, it definitely shouldn't be doing that. Perhaps at some point > >>> >> it'll note when > >>> >> your existing list of reviewers doesn't cover all of your files, but > >>> >> for now (at least, > >>> >> as of when my patch lands) I think it'll only suggest reviewers if > >>> >> there > >>> >> aren't > >>> >> any at all. > >>> > > >>> > does it pull it from rietveld to stay in sync, just like what happens > >>> > with > >>> > the description? > >>> > >>> Yes. > >>> > >>> > my use case is that i might run gcl change a number of times before > >>> > finally > >>> > uploading. if i remove that big list of reviewers, i would prefer > that > >>> > it > >>> > doesnt keep getting added. > >>> > >>> If you run change after the initial upload, it should insert "R=" plus > >>> the actual > >>> list of reviewers from the change. It should only ever suggest > >>> reviewers if there > >>> aren't any in the change. > >> > >> that's what I don't like though. i.e. I run gcl change to create the > list > >> initially, and remove the list of reviewers. then each time i run gcl > >> change it adds them. can this only be done on change creation? > > > > You're talking about running 'gcl change' multiple times prior to 'gcl > upload'? > > > > Yeah, we could make the change such that we only suggested people on > > the initial creation of the issue. > > > > I could probably also add a --don't-suggest-any-reviewers sort of flag > > or config setting if you would prefer to just stop this > > nonsense once and for all. > > > > -- Dirk >
On Wed, Mar 23, 2011 at 5:24 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Wed, Mar 23, 2011 at 5:22 PM, Dirk Pranke <dpranke@chromium.org> wrote: >> >> On Wed, Mar 23, 2011 at 4:57 PM, John Abd-El-Malek <jam@chromium.org> >> wrote: >> > >> > >> > On Wed, Mar 23, 2011 at 3:52 PM, Dirk Pranke <dpranke@chromium.org> >> > wrote: >> >> >> >> On Wed, Mar 23, 2011 at 3:44 PM, John Abd-El-Malek <jam@chromium.org> >> >> wrote: >> >> > >> >> > >> >> > On Wed, Mar 23, 2011 at 3:36 PM, Dirk Pranke <dpranke@chromium.org> >> >> > wrote: >> >> >> >> >> >> On Wed, Mar 23, 2011 at 3:32 PM, John Abd-El-Malek >> >> >> <jam@chromium.org> >> >> >> wrote: >> >> >> > >> >> >> > >> >> >> > On Wed, Mar 23, 2011 at 3:11 PM, Dirk Pranke >> >> >> > <dpranke@chromium.org> >> >> >> > wrote: >> >> >> >> >> >> >> >> On Wed, Mar 23, 2011 at 3:03 PM, John Abd-El-Malek >> >> >> >> <jam@chromium.org> >> >> >> >> wrote: >> >> >> >> > when you readd this, please don't add it to the bottom of cl >> >> >> >> > descriptions. >> >> >> >> > can it be automatically sent to rietveld out-of-band, so that >> >> >> >> > the >> >> >> >> > cl >> >> >> >> > description doesn't include this? also, it would be good to >> >> >> >> > not >> >> >> >> > keep >> >> >> >> > readding this if i remove it. >> >> >> >> >> >> >> >> It is my intent so that the R= shows up in your $EDITOR but is >> >> >> >> stripped from the description on sending to Rietveld (and the >> >> >> >> reviewers get set correctly in Rietveld instead). >> >> >> > >> >> >> > ah ok, that sounds perfect >> >> >> > >> >> >> >> >> >> >> >> I think it doesn't quite work correctly at the moment but I have >> >> >> >> another patch pending that should fix it. >> >> >> >> >> >> >> >> I'm not sure if that's quite what you're asking for. My thinking >> >> >> >> is >> >> >> >> that I always want the reviewers to be displayed when I have the >> >> >> >> change open in the editor so I can see who it will be set to and >> >> >> >> change it inline as necessary. >> >> >> >> >> >> >> >> Are you looking for something else? >> >> >> > >> >> >> > right now when i touch a bunch of files, i get a huge list of >> >> >> > reviewers. >> >> >> > if >> >> >> > i edit it, i dont want to have that change lost each time i run >> >> >> > gcl >> >> >> > change >> >> >> > to add one file.. >> >> >> >> >> >> Right, it definitely shouldn't be doing that. Perhaps at some point >> >> >> it'll note when >> >> >> your existing list of reviewers doesn't cover all of your files, but >> >> >> for now (at least, >> >> >> as of when my patch lands) I think it'll only suggest reviewers if >> >> >> there >> >> >> aren't >> >> >> any at all. >> >> > >> >> > does it pull it from rietveld to stay in sync, just like what happens >> >> > with >> >> > the description? >> >> >> >> Yes. >> >> >> >> > my use case is that i might run gcl change a number of times before >> >> > finally >> >> > uploading. if i remove that big list of reviewers, i would prefer >> >> > that >> >> > it >> >> > doesnt keep getting added. >> >> >> >> If you run change after the initial upload, it should insert "R=" plus >> >> the actual >> >> list of reviewers from the change. It should only ever suggest >> >> reviewers if there >> >> aren't any in the change. >> > >> > that's what I don't like though. i.e. I run gcl change to create the >> > list >> > initially, and remove the list of reviewers. then each time i run gcl >> > change it adds them. can this only be done on change creation? >> >> You're talking about running 'gcl change' multiple times prior to 'gcl >> upload'? > > yep > >> >> Yeah, we could make the change such that we only suggested people on >> the initial creation of the issue. > > i think this is good. it's like how we only add files to a cl the first > time it's created, and afterwards, the user has to move the modified files > above the magic marker. > Okay. |
