Adding uncommitted flag to support post staged but not commit changes

Review Request #6879 - Created Feb. 1, 2015 and updated

Tran Nguyen
RBTools
master
756835e...
rbtools

Problem:
While the common workflow is to post committed changes, some people have workflows where they want to post changes that are uncommitted, and have to resort to generating a diff manually and posting it using rbt post --diff-filename=.

This work would add an --uncommitted flag to rbt post that would generate diffs for uncommitted changes instead.

Approach:
Adding can_diff_committed capability to all clients, and let git set that to True to support it.
This revision simulate the behavior of svn, where we specify the tip to be --rbtools-working-copy so that git client know to remove the revision range out of the git commands.
In addition, to support this flag, --update, --diff-filename, and --guess-fields can't coexists, so the error is thrown when the user specifies those commands along with --uncommitted.
In order to view staged but uncommitted files, we need to add --cached to the git diff command.

Write an extra unit test to check that the git post --uncommitted works.
Verify that it throws error when --uncommitted and other options like --diff-filename,

  • 2
  • 0
  • 105
  • 4
  • 111
Description From Last Updated
Add another blank line here. David Trowbridge David Trowbridge
This is useless (we just set result to be an empty dict above -- so there will never be a ... David Trowbridge David Trowbridge
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 28
     E222 multiple spaces after operator
    
  3. rbtools/clients/tests.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (90 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 1)
     
     
    Col: 43
     E203 whitespace before ':'
    
  5. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  6. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 37
     E203 whitespace before ':'
    
  7. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  8. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (90 > 79 characters)
    
  9. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (96 > 79 characters)
    
  10. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 77
     E225 missing whitespace around operator
    
  11. rbtools/commands/post.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (93 > 79 characters)
    
  12. 
      
Tran Nguyen
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/git.py (Diff revision 2)
     
     
    Col: 60
     E251 unexpected spaces around keyword / parameter equals
    
  3. rbtools/clients/git.py (Diff revision 2)
     
     
    Col: 62
     E251 unexpected spaces around keyword / parameter equals
    
  4. rbtools/commands/post.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (94 > 79 characters)
    
  5. rbtools/utils/review_request.py (Diff revision 2)
     
     
    Col: 46
     E251 unexpected spaces around keyword / parameter equals
    
  6. rbtools/utils/review_request.py (Diff revision 2)
     
     
    Col: 48
     E251 unexpected spaces around keyword / parameter equals
    
  7. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. rbtools/utils/review_request.py (Diff revision 3)
     
     
    Col: 46
     E251 unexpected spaces around keyword / parameter equals
    
  3. rbtools/utils/review_request.py (Diff revision 3)
     
     
    Col: 48
     E251 unexpected spaces around keyword / parameter equals
    
  4. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. 
      
David Trowbridge
  1. This looks like a pretty good start!

  2. rbtools/clients/git.py (Diff revision 4)
     
     

    Comments should be full sentences with proper capitalization and punctuation.

  3. rbtools/clients/git.py (Diff revision 4)
     
     
     
     
     

    Given that we now do this in both the n_revs == 0 and n_revs >= 1 cases, perhaps we should move it out to happen just before we return result?

  4. rbtools/clients/git.py (Diff revision 4)
     
     

    We should probably not show this warning if using this flag.

  5. rbtools/clients/git.py (Diff revision 4)
     
     
     
     

    Hmm. So I can see people wanting both the diff in the working copy (unstaged) and the diff in the index (staged). Perhaps we should make --uncommitted be able to take an optional argument?

    Please also make sure that this comment is a full sentence with proper capitalization and punctuation.

    1. Can you clarify the format of the optinal argument? Is it going to be a list of unstaged files or a boolean to show all unstaged files?

      I think the former is preferable because a project might have a lot of artifact that hasn't been added to .gitignore. Some of these might not be of interest to the user, but I want to make sure I understand the requirement.

    2. Well, I'm thinking maybe it can be an enumerated value rather than boolean. I can see perhaps wanting --uncommitted=workingcopy and --uncommitted=staged (and maybe have it so --uncommitted without a value defaults to workingcopy and the lack of --uncommitted makes it use the current behavior).

    3. I'm not sure there is a clean way to default to workingcopy, so I let the user specify this option either with workingcopy or staged. I add the appropriate test for it.

  6. rbtools/clients/git.py (Diff revision 4)
     
     

    Please add this line back in.

  7. rbtools/clients/git.py (Diff revision 4)
     
     
     

    Please fix this comment.

  8. rbtools/clients/git.py (Diff revision 4)
     
     
     

    Add a blank line between these two.

  9. rbtools/clients/git.py (Diff revision 4)
     
     

    Leftover debugging print?

  10. rbtools/clients/tests.py (Diff revision 4)
     
     

    "GitClient". Also this test name is pretty hard to read (perhaps "Testing GitClient uncommitted diff with staged files"?) Because nose will append an ellipsis, we don't put periods at the end of test case docstrings.

  11. rbtools/commands/post.py (Diff revision 4)
     
     
     
     
     
     
     
     
     

    This error message is kind of hard to read. Maybe we can have separate errors for each of the conflicting options?

  12. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/git.py (Diff revision 6)
     
     
    Col: 22
     W601 .has_key() is deprecated, use 'in'
    
  3. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. 
      
David Trowbridge
  1. 
      
  2. rbtools/clients/git.py (Diff revision 8)
     
     

    We should default this to None rather than the empty string.

  3. rbtools/clients/git.py (Diff revision 8)
     
     

    Reading this through, I think we should keep the handling of the uncommitted flag in this conditional:

    if uncommitted is not None:
        del result['commit_id']
        result['base'] = self._rev_parse(self.get_head_ref())[0]
    
        if uncommitted == 'workingcopy':
            result['tip'] = self.REVISION_WORKING_COPY
        elif uncommitted == 'staged':
            result['tip'] = self.REVISION_STAGED
        else:
            assertion?
    elif n_revs == 0:
        ...
    
  4. rbtools/clients/git.py (Diff revision 8)
     
     
     
     
     
     

    We're still duplicating this code. If you make the above change, you can just have these lines run all the time before return result

  5. rbtools/clients/git.py (Diff revision 8)
     
     

    Typo: "Otherise". Also, add a period at the end.

  6. rbtools/clients/git.py (Diff revision 8)
     
     
     

    Add a blank line between these two.

  7. rbtools/clients/git.py (Diff revision 8)
     
     

    Add a period at the end.

  8. rbtools/clients/tests.py (Diff revision 8)
     
     

    Can we test the parse_revision_spec logic here?

  9. rbtools/clients/tests.py (Diff revision 8)
     
     

    Can we test the parse_revision_spec logic here?

  10. rbtools/commands/post.py (Diff revision 8)
     
     

    Remove this line.

  11. rbtools/commands/post.py (Diff revision 8)
     
     

    Can we specify choices in here?

  12. rbtools/commands/post.py (Diff revision 8)
     
     

    Typo: an -> and. You also have an extra ) in the string.

  13. rbtools/commands/post.py (Diff revision 8)
     
     

    Typo: an -> and. You also have an extra ) in the string.

  14. rbtools/commands/post.py (Diff revision 8)
     
     
     

    Typo: an -> and. You also have an extra ) in the string.

  15. rbtools/utils/review_request.py (Diff revision 8)
     
     

    This should probably default to None.

  16. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/tests.py (Diff revision 9)
     
     
    Col: 55
     E231 missing whitespace after ','
    
  3. rbtools/clients/tests.py (Diff revision 9)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 9)
     
     
    Col: 55
     E231 missing whitespace after ','
    
  5. 
      
Tran Nguyen
Tran Nguyen
David Trowbridge
  1. 
      
  2. rbtools/clients/git.py (Diff revision 10)
     
     
     
     

    Do you mind sorting these in alphabetical order?

  3. rbtools/clients/git.py (Diff revision 10)
     
     
     
     
     

    Can you add blank lines around the blocks? We should also use del rather than .pop().

    Please also use exact comparisons rather than 'in' (which does a substring match, which is slower and imprecise).

    if 'commit_id' in result:
        del result['commit_id']
    
    result['base'] = self._rev_parse(self.get_head_red())[0]
    
    if uncommitted == 'workingcopy':
        ...
    
  4. rbtools/clients/git.py (Diff revision 10)
     
     

    This change shouldn't be necessary anymore because the uncommitted handling is a separate case.

  5. rbtools/clients/git.py (Diff revision 10)
     
     

    Remove this blank line?

  6. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. rbtools/commands/post.py (Diff revision 11)
     
     
    Col: 48
     E127 continuation line over-indented for visual indent
    
  3. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. 
      
David Trowbridge
  1. 
      
  2. rbtools/commands/post.py (Diff revision 12)
     
     
     
     

    For the help, how about "Uploads a diff of the changes in either the working copy or index (Git only)."

  3. rbtools/commands/post.py (Diff revision 12)
     
     
     
     
     
     
     
     
     
     
     
     

    Why are these not allowed?

    1. I think we discussed this the last time about update, but I don't remember exactly the reason. For the exclude_pattern, it's currently not supported because there is not a clean way to support it based on my discussion with Barret

    2. Update is probably because it guesses based on the commit message (and of course there isn't one). We should probably make it so that update can work without a commit message and just try guessing all open review requests in turn.

      That said, the comments next to these shouldn't say what the code does (the code is very simple--I can read it easily), but rather, answer this question (why). That way if circumstances change later, we will know that we can change the restrictions here.

    3. So for the guesses, do you want me to support that in this feature or I should open it as a seperate issue?

    4. I think we should get this change ready as-is, and you can work on supporting -u in a separate review request.

  4. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/utils/review_request.py
        rbtools/clients/git.py
    
    
  2. 
      
Christian Hammond
  1. So my main concern is that passing --uncommitted=staged or --uncommitted=workingcopy is a lot to type every time, and perhaps is confusing to people without reading docs. How about instead having --uncommitted as a boolean flag, and then --uncommitted-type=[staged|workingcopy], with the default of "staged"?

    The pro being that, in the common case of posting changes scheduled for commit but not yet committed, it's quicker to type and post. Also, --uncommitted-type could be a configurable option, so that the user can specify which mode they prefer to work with in .reviewboardrc without triggering uncommitted reviews for every change.

    Downside being that, in the uncommon case, it's more to type, but I think most people just want to post changes scheduled for commit without actually committing. However, with the option, that shouldn't be as big a deal.

  2. rbtools/clients/git.py (Diff revision 14)
     
     
     

    Single quotes are preferred.

  3. rbtools/clients/git.py (Diff revision 14)
     
     
     

    Minor nit: This wraps kind of early.

  4. rbtools/clients/git.py (Diff revision 14)
     
     

    Can you flip this? Should be uncommitted == 'workingcopy'

  5. rbtools/clients/git.py (Diff revision 14)
     
     

    Same here.

  6. rbtools/clients/git.py (Diff revision 14)
     
     

    This should probably be given an error message indicating that uncommitted has an incorrect value?

  7. rbtools/clients/git.py (Diff revision 14)
     
     

    There's some important stuff in git_cmd we don't want to lose, like turning off quoted paths nad colors.

  8. rbtools/clients/git.py (Diff revision 14)
     
     

    Best to use tuples here, to avoid the overhead of a list.

  9. rbtools/clients/git.py (Diff revision 14)
     
     

    Best to use .append() here.

  10. rbtools/clients/git.py (Diff revision 14)
     
     
     

    That line shouldn't be indented.

  11. rbtools/clients/tests.py (Diff revision 14)
     
     

    Missing period.

  12. rbtools/clients/tests.py (Diff revision 14)
     
     

    Missing period.

  13. rbtools/clients/tests.py (Diff revision 14)
     
     
     

    Summaries are limited to one line. No wrapping.

  14. rbtools/commands/post.py (Diff revision 14)
     
     

    No need for this line. It's implied.

  15. rbtools/commands/post.py (Diff revision 14)
     
     
     
     
     

    Text must be aligned.

  16. rbtools/commands/post.py (Diff revision 14)
     
     

    Maybe just check for uncommitted once, and then do the rest of the conditionals inside that if block.

  17. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/errors.py (Diff revision 15)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. rbtools/clients/git.py (Diff revision 15)
     
     
     'UncommittedIncorrectValueError' imported but unused
    
  4. rbtools/clients/git.py (Diff revision 15)
     
     
     undefined name 'OptionsCheckError'
    
  5. rbtools/clients/git.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  6. rbtools/commands/post.py (Diff revision 15)
     
     
    Col: 32
     E128 continuation line under-indented for visual indent
    
  7. rbtools/commands/post.py (Diff revision 15)
     
     
    Col: 13
     E303 too many blank lines (2)
    
  8. rbtools/commands/post.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  9. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. 
      
David Trowbridge
  1. 
      
  2. rbtools/clients/errors.py (Diff revision 16)
     
     
     
     

    This is short enough that you should be able to put the """s on the same line as the docstring itself. There should also be a period at the end.

  3. rbtools/clients/git.py (Diff revision 16)
     
     
     

    This comment isn't really useful (the entire function is figuring out tip/base/parent_base), and kind of confusing. I'd just get rid of it.

  4. rbtools/clients/git.py (Diff revision 16)
     
     
     

    Add a blank line between here.

  5. rbtools/clients/git.py (Diff revision 16)
     
     
     

    This comment isn't 100% accurate. Maybe say something about how we add the revision range to the diff command when posting committed changes?

  6. rbtools/commands/post.py (Diff revision 16)
     
     
     

    The second sentence here isn't good grammar. How about "This is because the update mechanism uses the commit message to find the review request, and uncommitted changes don't have a commit message yet."

  7. rbtools/commands/post.py (Diff revision 16)
     
     
     
     

    Please figure out why it's not supported and explain that here. You also have a typo ("exlude")

  8. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. 
      
David Trowbridge
  1. Your description should be updated to talk about --uncommitted-type. You also have a typo ("Mercurila").

  2. rbtools/clients/git.py (Diff revision 17)
     
     
     

    This comment is very self-referential to the code, and doesn't really add a lot. How about:

    If the user passed --uncommitted-type=staged, add --cached to the 'git diff' command.

  3. rbtools/clients/git.py (Diff revision 17)
     
     
     

    Similar to the comment above, this is kind of confusing. How about:

    If the user asked for a diff of the working copy, we do not pass any revisions to the 'git diff' command. Otherwise, add the revision range.

  4. 
      
Tran Nguyen
Review Bot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. 
      
Barret Rennie
  1. 
      
  2. rbtools/clients/errors.py (Diff revision 18)
     
     
     
     

    You should probably just use CommandError instead of this.

    1. Is it too generic though? The OptionCheckError in the same file seems to be a CommandError but it has its own definition.

    2. The OptionCheckError would be fine, too.

  3. rbtools/clients/tests.py (Diff revision 18)
     
     

    Since this doesn't actually interface with Git, I would suggest renaming this to _create_file or _write_file.

  4. rbtools/commands/post.py (Diff revision 18)
     
     
     
     
     
     
     
     
     

    Maybe instead of the --uncommitted-type flag you could have a --staged flag that would work as follows:

    If --uncommitted flag is specified and --staged isn't specified it will use the working copy changes; otherwise if --staged is specified, it will use the staged changes.

    This seems like a clearer way to go about it (and will be easier to describe).

    1. Having these two flags was Christian's suggestion. How about you and he hash it out, so we don't have Tran thrash around too much between different options?

    2. I just followed what Christian suggested in previous post which I quoted "So my main concern is that passing --uncommitted=staged or --uncommitted=workingcopy is a lot to type every time, and perhaps is confusing to people without reading docs. How about instead having --uncommitted as a boolean flag, and then --uncommitted-type=[staged|workingcopy], with the default of "staged"?" . I get your point, but I feel like I keep going back and forth fixing cosmetic issues everytime someone looks at it which not really help me learn. Is there a guideline or standard where I could refer to when applying my changes?

    3. This is just a suggesstion, so feel free to drop it if you prefer Christian's suggestion. Either way is fine for me.

    4. I'm happy enough with Barret's suggestion. It's cleaner.

  5. rbtools/commands/post.py (Diff revision 18)
     
     
     

    Why this change?

  6. rbtools/commands/post.py (Diff revision 18)
     
     
     

    Blank line between these.

  7. rbtools/commands/post.py (Diff revision 18)
     
     
     

    Blank line between these.

  8. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. 
      
Tran Nguyen
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. rbtools/commands/post.py (Diff revision 20)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. 
      
Christian Hammond
  1. 
      
  2. rbtools/clients/git.py (Diff revision 21)
     
     
     
     
     

    I'm not sure we want to pass a string for this value. It's too easy to typo, and isn't very obvious to the caller.

    What I'd suggest instead is a class that works as an enum for storing the possible values. For instance:

    class UncommittedMode(object):
        WORKING_COPY = 1
        STAGED = 2
    

    Then it's easy to check.

  3. rbtools/clients/git.py (Diff revision 21)
     
     

    I don't see anywhere where this particular exception is caught.

    The name is a bit awkward. How about InvalidUncommittedModeError, to match the suggested class name?

    1. "I don't see anywhere where this particular exception is caught" > Can you elaborate on this? Are you talking about trying to catch it somewhere so that the user can continue?

    2. I think what he means is, in commands/diff.py

          try:
              revisions = tool.parse_revision_spec(args)
              extra_args = None
          except InvalidRevisionSpecError:
      

      there should be another 'except' to catch the UncommittedIncorrectValueError

    3. Well, that makes sense, but what protocol to do here except raise the exception again? I don't see any particular way to handle this exception since the revisions variables is not complete.

  4. rbtools/clients/git.py (Diff revision 21)
     
     
     

    This should be:

    if tip not in (self.REVISION_WORKING_COPY, self.REVISION_STAGED):
    

    No need to store as a list and then convert to a tuple.

  5. rbtools/commands/post.py (Diff revision 21)
     
     

    "... in the index. Use ..."

    Note the period and "the".

  6. rbtools/commands/post.py (Diff revision 21)
     
     
     

    Comma should go immediately after the word it follows, within the same string.

    Also, should be "--staged to upload the staged changes."

  7. rbtools/commands/post.py (Diff revision 21)
     
     

    "... to the working copy."

  8. rbtools/commands/post.py (Diff revision 21)
     
     
     
     
     

    "and this option is specified, a diff of the staged changes will be uploaded. Otherwise, the changed in the working copy are uploaded."

  9. rbtools/commands/post.py (Diff revision 21)
     
     
     

    This comment just reflects the code exactly, so it doesn't need to be here.

  10. rbtools/commands/post.py (Diff revision 21)
     
     
  11. rbtools/commands/post.py (Diff revision 21)
     
     
     
     
     

    Some grammatical fixes:

    "If the user has explicitly specified the --uncommitted option, then the --update option cannot be used."

  12. rbtools/commands/post.py (Diff revision 21)
     
     
  13. rbtools/commands/post.py (Diff revision 21)
     
     
     
     
     
     
     

    Some grammatical fixes:

    "If the user has explicitly specified the --uncommitted option, then the --exclude-patterns option cannot be used."

    The note about diff-tree is a Git-specific implementation detail, so we shouldn't mention it here.

    1. Previous mentors mentioned that I need to indicate the reason for not supporting this and the main reason is to find an alternative to diff-tree command for git. If we remove it, how should I rephrase this to satisfy the requirement of the comment?

  14. rbtools/commands/post.py (Diff revision 21)
     
     

    "exclude-patterns", not "exclude_patterns".

  15. rbtools/commands/post.py (Diff revision 21)
     
     
  16. 
      
Tien Vu
  1. 
      
  2. rbtools/clients/tests.py (Diff revision 21)
     
     

    this seems redundant

    self.assertEqual(result['parent_diff'], None)

    implies

    self.assertTrue('parent_diff' in result)

    same for the base_commit_id one

    1. I don't think this comment is valid though, if you try to do "self.assertEqual(result['parent_diff'], None)" without making sure that result actually has 'parent_diff', you will have ValueError exception because you try to access an item that not in the dictionary yet.

    2. yeah, I was thinking that if the key wasn't in the dict, the test will fail anyway. but an exception is probably not as good as an explicit assertion

  3. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/git.py (Diff revision 22)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. rbtools/clients/git.py (Diff revision 22)
     
     
    Col: 54
     E231 missing whitespace after ','
    
  4. 
      
Tran Nguyen
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. 
      
Barret Rennie
  1. 
      
  2. rbtools/clients/git.py (Diff revision 23)
     
     

    This comment needs to be updated to reflect the new flags.

  3. rbtools/clients/tests.py (Diff revision 23)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can we rename file to filename in all these instances? file is overriding a Python builtin.

  4. rbtools/commands/post.py (Diff revision 23)
     
     
     

    Should --staged be shorthand for --staged --uncommitted ?

    1. In the previous comment, you mentioned that I should use --uncommitted to indicate changes in working copy and --uncommitted --staged to indicate staged change. For me, it makes senses that way because it's part of supporting uncommitted flag.

  5. 
      
Tran Nguyen
Review request changed

Commit:

-3860f9757e56c95feeceba83acff9f004c6f950c
+756835ef2152429dc74743f955cb274bf64e9b1a

Diff:

Revision 24 (+154 -20)

Show changes

Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/utils/review_request.py
        rbtools/clients/errors.py
        rbtools/clients/git.py
    
    
  2. 
      
David Trowbridge
  1. 
      
  2. rbtools/clients/git.py (Diff revision 24)
     
     

    Add another blank line here.

  3. rbtools/clients/git.py (Diff revision 24)
     
     
     

    This is useless (we just set result to be an empty dict above -- so there will never be a commit_id in it).

  4. 
      
Loading...