Adding uncommitted flag to support post staged but not commit changes

Review Request #6879 — Created Feb. 1, 2015 and discarded

Information

RBTools
master

Reviewers

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,

Description From Last Updated

Col: 28 E222 multiple spaces after operator

reviewbotreviewbot

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

reviewbotreviewbot

Col: 43 E203 whitespace before ':'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 37 E203 whitespace before ':'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 77 E225 missing whitespace around operator

reviewbotreviewbot

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

reviewbotreviewbot

Col: 60 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 62 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

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

reviewbotreviewbot

Col: 46 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 48 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 46 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 48 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Comments should be full sentences with proper capitalization and punctuation.

daviddavid

Given that we now do this in both the n_revs == 0 and n_revs >= 1 cases, perhaps we should …

daviddavid

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

daviddavid

Hmm. So I can see people wanting both the diff in the working copy (unstaged) and the diff in the …

daviddavid

Please add this line back in.

daviddavid

Please fix this comment.

daviddavid

Add a blank line between these two.

daviddavid

Leftover debugging print?

daviddavid

"GitClient". Also this test name is pretty hard to read (perhaps "Testing GitClient uncommitted diff with staged files"?) Because nose …

daviddavid

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

daviddavid

Col: 22 W601 .has_key() is deprecated, use 'in'

reviewbotreviewbot

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Add a blank line between these two.

daviddavid

Add a period at the end.

daviddavid

Can we test the parse_revision_spec logic here?

daviddavid

Can we test the parse_revision_spec logic here?

daviddavid

Remove this line.

daviddavid

Can we specify choices in here?

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

This should probably default to None.

daviddavid

Col: 55 E231 missing whitespace after ','

reviewbotreviewbot

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

reviewbotreviewbot

Col: 55 E231 missing whitespace after ','

reviewbotreviewbot

Do you mind sorting these in alphabetical order?

daviddavid

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

daviddavid

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

daviddavid

Remove this blank line?

daviddavid

Col: 48 E127 continuation line over-indented for visual indent

reviewbotreviewbot

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

daviddavid

Why are these not allowed?

daviddavid

Single quotes are preferred.

chipx86chipx86

Minor nit: This wraps kind of early.

chipx86chipx86

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

chipx86chipx86

Same here.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Best to use .append() here.

chipx86chipx86

That line shouldn't be indented.

chipx86chipx86

Missing period.

chipx86chipx86

Missing period.

chipx86chipx86

Summaries are limited to one line. No wrapping.

chipx86chipx86

No need for this line. It's implied.

chipx86chipx86

Text must be aligned.

chipx86chipx86

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

chipx86chipx86

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

'UncommittedIncorrectValueError' imported but unused

reviewbotreviewbot

undefined name 'OptionsCheckError'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 32 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

This is short enough that you should be able to put the """s on the same line as the docstring …

daviddavid

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 …

daviddavid

Add a blank line between here.

daviddavid

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

daviddavid

The second sentence here isn't good grammar. How about "This is because the update mechanism uses the commit message to …

daviddavid

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

daviddavid

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

daviddavid

Similar to the comment above, this is kind of confusing. How about: If the user asked for a diff of …

daviddavid

You should probably just use CommandError instead of this.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Why this change?

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

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

reviewbotreviewbot

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

chipx86chipx86

I don't see anywhere where this particular exception is caught. The name is a bit awkward. How about InvalidUncommittedModeError, to …

chipx86chipx86

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 …

chipx86chipx86

this seems redundant self.assertEqual(result['parent_diff'], None) implies self.assertTrue('parent_diff' in result) same for the base_commit_id one

TI tienv

"... in the index. Use ..." Note the period and "the".

chipx86chipx86

Comma should go immediately after the word it follows, within the same string. Also, should be "--staged to upload the …

chipx86chipx86

"... to the working copy."

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

No \n.

chipx86chipx86

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

chipx86chipx86

No \n.

chipx86chipx86

Some grammatical fixes: "If the user has explicitly specified the --uncommitted option, then the --exclude-patterns option cannot be used." The …

chipx86chipx86

"exclude-patterns", not "exclude_patterns".

chipx86chipx86

No \n.

chipx86chipx86

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 54 E231 missing whitespace after ','

reviewbotreviewbot

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Add another blank line here.

daviddavid

This is useless (we just set result to be an empty dict above -- so there will never be a …

daviddavid
reviewbot
  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. 
      
VI
VI
reviewbot
  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. 
      
VI
reviewbot
  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. 
      
VI
reviewbot
  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
  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. 
      
VI
reviewbot
  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. 
      
VI
reviewbot
  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. 
      
VI
reviewbot
  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. 
      
VI
reviewbot
  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
  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. 
      
VI
reviewbot
  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. 
      
VI
VI
david
  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. 
      
VI
reviewbot
  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. 
      
VI
reviewbot
  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
  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. 
      
VI
reviewbot
  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. 
      
VI
reviewbot
  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. 
      
chipx86
  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. 
      
VI
reviewbot
  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. 
      
VI
reviewbot
  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
  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. 
      
VI
reviewbot
  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
  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. 
      
VI
reviewbot
  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. 
      
brennie
  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. 
      
VI
reviewbot
  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. 
      
VI
VI
reviewbot
  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. 
      
VI
reviewbot
  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. 
      
chipx86
  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. 
      
TI
  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. 
      
VI
reviewbot
  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. 
      
VI
reviewbot
  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. 
      
brennie
  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. 
      
VI
reviewbot
  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
  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. 
      
david
Review request changed

Status: Discarded

Loading...