Issue 127290043: Log when a patchset is deleted and when reviewers change (Closed)
DescriptionLog when a patchset is deleted and when reviewers change.
For patchset deletion messages, look at the last two messages here:
https://skia-codereview-staging.appspot.com/341001/
For reviewer change messages, look at the messages here:
https://skia-codereview-staging.appspot.com/2821001/
BUGS=
* https://code.google.com/p/chromium/issues/detail?id=403381 ('Log when a patchset is deleted in Rietveld')
* https://code.google.com/p/chromium/issues/detail?id=403777 ('Log when the reviewers list in Rietveld changes')
Patch Set 1 : Log when patchset is deleted #Patch Set 2 : Log when reviewers change #
Total comments: 2
Patch Set 3 : Display additions and removals #
Total comments: 5
Patch Set 4 : Address comments #Patch Set 5 : Make sure nothing changed locally #MessagesTotal messages: 13
https://codereview.appspot.com/127290043/diff/20001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/127290043/diff/20001/codereview/views.py#newco... codereview/views.py:1635: # The list of reviewers have changed, log what changed and who changed it. Please spell out additions and removals to highlight what changed. E.g., additions_set = orig_reviewers_set - new_reviewers_set removals_set = new_reviwers_set - orig_reviewers_set Then format the message with just +addition and -removal for each item.
Sign in to reply to this message.
https://codereview.appspot.com/127290043/diff/20001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/127290043/diff/20001/codereview/views.py#newco... codereview/views.py:1635: # The list of reviewers have changed, log what changed and who changed it. On 2014/08/14 18:40:10, jrobbins (corp) wrote: > Please spell out additions and removals to highlight what changed. E.g., > additions_set = orig_reviewers_set - new_reviewers_set > removals_set = new_reviwers_set - orig_reviewers_set > > Then format the message with just +addition and -removal for each item. Done. Please try it out in https://skia-codereview-staging.appspot.com/781001/ (or some other issue in skia-codereview-staging).
Sign in to reply to this message.
lgtm after considering these comments. Be sure to remove the extra space before comma in two places. https://codereview.appspot.com/127290043/diff/40001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/127290043/diff/40001/codereview/views.py#newco... codereview/views.py:1640: reviewers_msg = '%s changed the reviewers list:\n' % ( Maybe shorten this to just "%s changed reviewers:". The reason is that the message will usually be viewed in a collapsed state and fitting more in one line would reduce the need for users to click to expand it. https://codereview.appspot.com/127290043/diff/40001/codereview/views.py#newco... codereview/views.py:1643: reviewers_msg += '+ %s\n' % ' , '.join(additions_set) Please remove the space before the comma, for both additions and removals. Using sorted(additions_set) would make the code determinate which would make testing easier later.
Sign in to reply to this message.
https://codereview.appspot.com/127290043/diff/40001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/127290043/diff/40001/codereview/views.py#newco... codereview/views.py:1643: reviewers_msg += '+ %s\n' % ' , '.join(additions_set) On 2014/08/15 19:10:54, jrobbins (corp) wrote: > Please remove the space before the comma, for both additions and removals. I added spaces because otherwise the whole string is converted into a link, Eg: https://skia-codereview-staging.appspot.com/2821001/#msg11 > > Using sorted(additions_set) would make the code determinate which would make > testing easier later.
Sign in to reply to this message.
On Fri, Aug 15, 2014 at 12:13 PM, <rmistry@google.com> wrote: > I added spaces because otherwise the whole string is converted into a > link, Eg: https://skia-codereview-staging.appspot.com/2821001/#msg11 How about just a space after the comma, no space before the comma?
Sign in to reply to this message.
Please look at the last 3 comments here: https://skia-codereview-staging.appspot.com/2821001/ https://codereview.appspot.com/127290043/diff/40001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/127290043/diff/40001/codereview/views.py#newco... codereview/views.py:1640: reviewers_msg = '%s changed the reviewers list:\n' % ( On 2014/08/15 19:10:54, jrobbins (corp) wrote: > Maybe shorten this to just "%s changed reviewers:". > > The reason is that the message will usually be viewed in a collapsed state and > fitting more in one line would reduce the need for users to click to expand it. Done. https://codereview.appspot.com/127290043/diff/40001/codereview/views.py#newco... codereview/views.py:1643: reviewers_msg += '+ %s\n' % ' , '.join(additions_set) On 2014/08/15 19:10:54, jrobbins (corp) wrote: > Please remove the space before the comma, for both additions and removals. It works with the space after the comma. Done. > > Using sorted(additions_set) would make the code determinate which would make > testing easier later. Done.
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
Hi Jason, I am back from vacation tomorrow morning, do you mind if I submit this then and then push to production and make it the default version? or would you rather do it? Thanks
Sign in to reply to this message.
On 2014/08/20 18:00:43, rmistry wrote: > Hi Jason, > > I am back from vacation tomorrow morning, do you mind if I submit this then and > then push to production and make it the default version? or would you rather do > it? > > Thanks I'll do it. I have other changes that I want to land and deploy too.
Sign in to reply to this message.
On 2014/08/20 18:19:36, jrobbins (corp) wrote: > On 2014/08/20 18:00:43, rmistry wrote: > > Hi Jason, > > > > I am back from vacation tomorrow morning, do you mind if I submit this then > and > > then push to production and make it the default version? or would you rather > do > > it? > > > > Thanks > > I'll do it. I have other changes that I want to land and deploy too. Went ahead and committed as https://code.google.com/p/rietveld/source/detail?r=6edbef70cf3416158f6968011b... Let me know whenever you are ready to push. Thanks!
Sign in to reply to this message.
OK, I am planning on tomorrow (Thursday) morning. On Wed, Aug 20, 2014 at 11:42 AM, <rmistry@google.com> wrote: > On 2014/08/20 18:19:36, jrobbins (corp) wrote: > >> On 2014/08/20 18:00:43, rmistry wrote: >> > Hi Jason, >> > >> > I am back from vacation tomorrow morning, do you mind if I submit >> > this then > >> and >> > then push to production and make it the default version? or would >> > you rather > >> do >> > it? >> > >> > Thanks >> > > I'll do it. I have other changes that I want to land and deploy too. >> > > Went ahead and committed as > https://code.google.com/p/rietveld/source/detail?r= > 6edbef70cf3416158f6968011b2df768dfaf301c&name=chromium > > Let me know whenever you are ready to push. Thanks! > > https://codereview.appspot.com/127290043/ >
Sign in to reply to this message.
On 2014/08/20 18:51:40, jrobbins (corp) wrote: > OK, I am planning on tomorrow (Thursday) morning. That sounds perfect. Thanks. > > > On Wed, Aug 20, 2014 at 11:42 AM, <mailto:rmistry@google.com> wrote: > > > On 2014/08/20 18:19:36, jrobbins (corp) wrote: > > > >> On 2014/08/20 18:00:43, rmistry wrote: > >> > Hi Jason, > >> > > >> > I am back from vacation tomorrow morning, do you mind if I submit > >> > > this then > > > >> and > >> > then push to production and make it the default version? or would > >> > > you rather > > > >> do > >> > it? > >> > > >> > Thanks > >> > > > > I'll do it. I have other changes that I want to land and deploy too. > >> > > > > Went ahead and committed as > > https://code.google.com/p/rietveld/source/detail?r= > > 6edbef70cf3416158f6968011b2df768dfaf301c&name=chromium > > > > Let me know whenever you are ready to push. Thanks! > > > > https://codereview.appspot.com/127290043/ > >
Sign in to reply to this message.
|
