Add a flag in rbt post to auto-stamp the review while posting

Review Request #7250 — Created April 24, 2015 and submitted

Information

RBTools
master
448ed5f...

Reviewers

This optional flag allows users to streamline their workflow by posting and
stamping their review with a single command. Instead of running rbt post and
then rbt stamp, it can be done all at once with rbt post -s.

  • Posted some reviews using rbt post -s under Git and Perforce.
  • Tried the error flows to make sure that posting will still happen even if stamping fails.
Description From Last Updated

Col: 15 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 19 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 19 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 15 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 19 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 15 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 19 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 19 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 15 E114 indentation is not a multiple of four (comment)

reviewbotreviewbot

Col: 15 E114 indentation is not a multiple of four (comment)

reviewbotreviewbot

Col: 15 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Please undo this line (only one blank line between import sections)

daviddavid

This is not necessarily true (I could easily see a case where my commit modifies one file and then a …

daviddavid

Do we really want to include the stamp in the review request description? Having a URL to itself doesn't seem …

daviddavid

Col: 11 E111 indentation is not a multiple of four

reviewbotreviewbot
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
  2. 
      
DR
  1. Is this change something that you think would be a useful addition to rbt? I know it's been discussed before in the Google Code.

    1. Yes, I think it is. We just haven't had time in the last few days to take a look.

  2. 
      
DR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
  2. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 15
     E111 indentation is not a multiple of four
    
    1. Oops. My editor was set to two spaces for some reason.

  3. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 19
     E111 indentation is not a multiple of four
    
  4. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 19
     E111 indentation is not a multiple of four
    
  5. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 15
     E111 indentation is not a multiple of four
    
  6. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 19
     E111 indentation is not a multiple of four
    
  7. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 15
     E111 indentation is not a multiple of four
    
  8. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 19
     E111 indentation is not a multiple of four
    
  9. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 19
     E111 indentation is not a multiple of four
    
  10. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 15
     E114 indentation is not a multiple of four (comment)
    
  11. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 15
     E114 indentation is not a multiple of four (comment)
    
  12. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 15
     E111 indentation is not a multiple of four
    
  13. 
      
DR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
  2. rbtools/commands/post.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
DR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/post.py (Diff revision 4)
     
     
    Show all issues

    Please undo this line (only one blank line between import sections)

  3. rbtools/commands/post.py (Diff revision 4)
     
     
     
     
     
    Show all issues

    This is not necessarily true (I could easily see a case where my commit modifies one file and then a bunch of other auto-generated junk that I don't care to post for review). If the user does both -s and -I, let's trust them.

    1. The only SCMs I'm really familiar with are Perforce and Git; I was coming at this from a Perforce standpoint, where if the user includes files on an ad hoc basis, rbt won't know which changelist to stamp. It's a question of practicality: if a user has decided to include a single file from each of two different changelists, which one should be stamped?

      Ideally, rbt post -s should behave [almost] exactly the same as rbt post && rbt stamp. So what should happen if someone tries to stamp onto individual files in Perforce (since -I is a global option)? I didn't actually test that out when I added stamping for Perforce, but to me the only somewhat-reasonable thing to do is to stamp every changelist that contains a file in the reviewed set, which I don't particularly like. Maybe it should stamp, but only if the file set is all under a single changelist?

      In Git, where we can just stamp HEAD no matter what, this isn't so much an issue because there's only one "active" commit at any given time, so we can probably safely let stamping work in that scenario.

    2. I just tested this out with Perforce. Here's what happens:
      - If I include a changelist and an ad-hoc file, rbt can't create a diff and exits.
      - If I include an ad-hoc file that's not in a changelist, rbt fails to stamp (because we can't stamp the default changelist), but posts the review anyway.
      - If I include an ad-hoc file that is in a changelist, rbt can't create a diff and exits.

      In Git, I can post a review with just the ad-hoc files, and rbt stamps my latest commit message. This seems like reasonable behavior.

      I guess I'm okay with letting the stamp go through, because it will fail anyway when it doesn't make sense.

  4. rbtools/commands/post.py (Diff revision 4)
     
     
     
     
     
     
    Show all issues

    Do we really want to include the stamp in the review request description? Having a URL to itself doesn't seem terribly useful.

    1. My motivation here was to smooth out the perhaps-unintended functionality of rbt post changelist_num under Perforce. When we want to update an existing review, we don't have to use -u, because rbt self-recovers from the "review request already exists for this change ID" error by changing the create to an update. Since the tool works reliably this way, it's inconvenient and unnecessary for developers to have to think about whether or not it's the first time they're submitting the code for review, and thus whether or not to add -u. If we just leave it off all the time, everything works fine (in my current organization, we have a GUI control in our tools to post a review from a changelist, so it's rather impossible -- or at least greatly cumbersome -- to try to figure out whether we should be creating or updating).

      I understand that rbt doesn't follow that paradigm with Git, but it does with Perforce whether you like it or not, and I suspect there are many people/organizations who currently use the tool in that fashion and would mourn the loss of convenience if it were to change. So I don't want to break it.

      Back to your question: No, I don't particularly think it's necessary to include the stamp in the review request description. However:

      1. It makes sense that, if we're guessing the review description from the commit description, that they match, and if I post and stamp in the same action, I would thus expect both things to be updated.

      2. The more important consideration: given the way I can "update" the review under perforce, if I post and stamp the review, then post it again, rbt will re-parse the changelist description and try to add the URL into the description on ReviewBoard. This means that ReviewBoard shows the description was changed in r2 even if the developer didn't manually modify the changelist description, consequently wasting space on the review page and confusing the reviewers by showing them a "changed description" that didn't really change in any meaningful way. So that's really what I'm trying to avoid.

      I can think of three potential ways to mitigate the second issue:

      1. Do what I did in this review

      2. Change the guessing logic to ignore the stamp string when parsing the summary and description (I don't like this because it might lead users to wonder why their entire commit message isn't getting parsed)

      3. Change the auto-recover logic so that when rbt finds out about the existing review for the given commit ID, it treats everything like an update (exactly as if the user had given -u) from that point forward, and would then not try to re-guess the summary/description unless the user explicitly specified those options when running the command.

    2. The more I think about it, I see option 3 as the best thing to do (it will make things work more nicely even if there's no stamping involved). But you guys should be the ones who ultimately decide that behavior.

    3. I tried to implement option 3, but it's somehow pulling the commit description anyway if I run rbt post cln again after stamping (looks like it's being caused by the review_request.update on 417-419, but I can't figure out why). I think it's going to be too risky to get this working and cover the edge cases correctly, so I'm sticking with my original approach (stamp, and then guess/parse/update the description fields).

      It looks like this Perforce stuff was originally implemented in this commit: https://github.com/reviewboard/rbtools/commit/5c889c93

  5. 
      
DR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
  2. rbtools/commands/post.py (Diff revision 5)
     
     
    Show all issues
    Col: 11
     E111 indentation is not a multiple of four
    
  3. 
      
DR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/utils/commands.py
    
    
  2. 
      
david
  1. Ship It!
    1. Thanks! Is there any documentation I should add in any other places, or will the doc gen tools get everything it needs from the option description?

    2. The docs should pick it up from the option description. The only thing I can think of that might be useful is a note in the rbt stamp docs about this feature.

    3. That does seem like a good idea. I'll add it.

  2. 
      
DR
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.7.x (f2c4ac3)