Summary

Adding in a smarter parent-base finding algorithm

Review Request #7985 — Created Feb. 21, 2016 and submitted Aug. 26, 2016, 11:29 a.m.

Information

Owner

ssengar

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

Main idea
Given a tree like this one: 
A - * - * - * - * origin/master

   * - B - * - * origin/feature

         - C - D topic
when we rbt post, it would like to have three things, 

            'base':        A revision to use as the base of the resulting diff. 

            'tip':         A revision to use as the tip of the resulting diff.

            'parent_base': (optional) The revision to use as the base of a     

                           parent diff.                                        

Now, parent_base has to be in the remote repository, in other words, a ancestor of a commit of any branch origin/*.  We would also like parent_base to be as young an ancestor of tip as possible, given the first constraint.
A git-rev-list query can find this in the following manner -
git rev-list $TIP --not --remotes=origin | tail -1
should give the oldest ancestor that is not reachable from a public branch, so it's parent will be the best parent base.

Testing Done

The algorithm has been tested extensively. More specifically, GitClient.parse_revision_spec has been tested on the conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). Detailed documentation and visualization have been added to the tests to aid understanding. I hope this will help a reader understand and review the tests better.

Specifically, the conditions being tested are -

0) Testing GitClient.parse_revision_spec with target branch off a tracking branch not aligned with the remote.
1) Testing GitClient.parse_revision_spec with target branch off a tracking branch aligned with the remote.
2) Testing GitClient.parse_revision_spec with target branch off a tracking branch with changes since the remote.
3) Testing GitClient.parse_revision_spec with target branch off a branch not properly tracking the remote.
4) Testing GitClient.parse_revision_spec with a target branch that merged a tracking branch off another tracking branch.
5) Testing GitClient.parse_revision_spec with a target branch posted off a tracking branch that merged another tracking branch.
6) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch without any tracking branches.
7) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch that is aligned to the same commit as another remote branch.
8) Testing GitClient.parse_revision_spec with a target branch not up-to-date with a remote branch.
9) Testing GitClient.parse_revision_spec with a target branch that has branches from different remotes in its path.

Note - The above test descriptions make more sense when we actually view the commit graphs representing each of these scenarios. These commit graphs can be found as part of the documentation of my tests.

Note 2 - The test functions are named in a serial manner (i.e. with suffixes case_one, case_two ... case_nine etc.). This was done because it was not always easy to capture what the test was trying to test in a few words. Hence giving uniform but serial names helped because essentially, all of these tests test the same thing - i.e. whether GitClient.parse_revision_spec can find a good parent, parent_base in each of these scenarios.

I also had to alter one of the existing tests in order to align it with the new algorithm.

Issues

Description	From	Last Updated
Col: 55 W291 trailing whitespace	reviewbot	Feb. 28, 2016, 7:28 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 28, 2016, 7:28 p.m.
Col: 54 E262 inline comment should start with '# '	reviewbot	Feb. 28, 2016, 7:28 p.m.
Col: 53 E261 at least two spaces before inline comment	reviewbot	Feb. 28, 2016, 7:28 p.m.
Col: 55 W291 trailing whitespace	reviewbot	Feb. 28, 2016, 7:28 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 28, 2016, 7:28 p.m.
Col: 55 W291 trailing whitespace	reviewbot	March 14, 2016, 12:46 p.m.
Col: 55 W291 trailing whitespace	reviewbot	March 12, 2016, 7:02 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	March 12, 2016, 7:02 p.m.
Col: 61 W291 trailing whitespace	reviewbot	March 12, 2016, 7:02 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	March 12, 2016, 7:02 p.m.
Col: 55 W291 trailing whitespace	reviewbot	March 12, 2016, 7:02 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 12, 2016, 7:02 p.m.
Since youre modifying the docstring for this function, could you reflow it into our document style. e.g. ``` Single line …	brennie	March 13, 2016, 12:16 p.m.
Needs a docstirng.	brennie	March 13, 2016, 12:16 p.m.
Prefer '--remotes=%s' % remote. String interpolation is faster than concatenation in general.	brennie	March 13, 2016, 12:16 p.m.
Missing a period. "at a commit" ?	brennie	March 13, 2016, 12:16 p.m.
Again prefer interpolation here.	brennie	March 13, 2016, 12:16 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 14, 2016, 12:47 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 13, 2016, 12:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 13, 2016, 12:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 13, 2016, 12:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 13, 2016, 12:17 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 13, 2016, 12:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 13, 2016, 12:17 p.m.
Col: 80 E501 line too long (113 > 79 characters)	reviewbot	March 13, 2016, 12:17 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 13, 2016, 12:17 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 13, 2016, 12:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 13, 2016, 12:17 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 13, 2016, 12:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 13, 2016, 12:17 p.m.
Col: 28 W291 trailing whitespace	reviewbot	March 14, 2016, 12:50 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 14, 2016, 12:50 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 14, 2016, 12:50 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 14, 2016, 12:50 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 14, 2016, 12:50 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 14, 2016, 12:50 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 14, 2016, 12:50 p.m.
Col: 80 E501 line too long (113 > 79 characters)	reviewbot	March 14, 2016, 12:50 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 14, 2016, 12:49 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 14, 2016, 12:49 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 14, 2016, 12:49 p.m.
"Parse"	brennie	March 20, 2016, 9:41 p.m.
Col: 28 W291 trailing whitespace	reviewbot	March 20, 2016, 9:40 p.m.
"A dictionary with the following keys:	brennie	March 20, 2016, 9:41 p.m.
Can you reformat this to be a reStructuredText definition list, e.g. ```python """ A dictionary with the following keys: ``'base'`` …	brennie	March 20, 2016, 9:41 p.m.
Docstrings should be in the following format: """Single line summary Multi-line description """ We also want to use the imperitive …	brennie	March 20, 2016, 9:42 p.m.
Single quotes for strings.	brennie	March 20, 2016, 9:42 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:17 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 20, 2016, 10:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:17 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:18 p.m.
Col: 80 E501 line too long (113 > 79 characters)	reviewbot	March 20, 2016, 9:41 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 20, 2016, 9:41 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:18 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:18 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:18 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	March 20, 2016, 9:41 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:18 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 20, 2016, 10:18 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:18 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 20, 2016, 9:41 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:18 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:19 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:19 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:19 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:19 p.m.
Col: 11 W291 trailing whitespace	reviewbot	March 20, 2016, 10:19 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:19 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:19 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:19 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:19 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 20, 2016, 10:19 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py

rbtools/clients/tests.py (Diff revision 1)
Show all issues
```
Col: 55
 W291 trailing whitespace
```

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

Show all issues

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

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

Show all issues

Col: 54
 E262 inline comment should start with '# '

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

Show all issues

Col: 53
 E261 at least two spaces before inline comment

rbtools/clients/tests.py (Diff revision 1)
Show all issues
```
Col: 55
 W291 trailing whitespace
```

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

Show all issues

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

Testing Done:

		The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.
	+
	+	I also had to alter one of the existing tests in order to align it with the new algorithm.

Diff:

Revision 2 (+228 -6)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py

rbtools/clients/tests.py (Diff revision 2)
Show all issues
```
Col: 55
 W291 trailing whitespace
```
rbtools/clients/tests.py (Diff revision 2)
Show all issues
```
Col: 55
 W291 trailing whitespace
```

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 2)
Show all issues
```
Col: 61
 W291 trailing whitespace
```

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 2)
Show all issues
```
Col: 55
 W291 trailing whitespace
```

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

Show all issues

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

Change Summary:

Updated test desciption and added unit tests

Testing Done:

		The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.

	+	Update - I have now written tests for conditions 3 through 7 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.
	+
	+	Note - As of now, the test functions are poorly named (i.e. case one, case two etc.). This is just to signal to the mentors which conditions they're trying to test. I will improve the names once the content of the test cases is apporved by the mentors.
	+
		I also had to alter one of the existing tests in order to align it with the new algorithm.

Testing Done:

		The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.

~		Update - I have now written tests for conditions 3 through 7 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.
	~	Update - I have now written tests for conditions 3 through 6 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.

		Note - As of now, the test functions are poorly named (i.e. case one, case two etc.). This is just to signal to the mentors which conditions they're trying to test. I will improve the names once the content of the test cases is apporved by the mentors.

		I also had to alter one of the existing tests in order to align it with the new algorithm.

Hey,

Thanks for putting these tests together. I know you wanted another pair of eyes on these, but given the complexity, I think I want more information shown in the tests in order to help with that. Basically, what a review will need for these tests is a visualization, and documentation.

I see that you have print statements for showing a graph of the resulting branches. We're not going to want those print statements in there for the final review, but I would like to see the output you get for those statements as comments in each test, showing what the tree actually looks like. Sort of in the form of what's in my review on /r/6797/.

That doesn't mean copy/paste from my review. Rather, I want to see the output you get, but formatted in a way that focuses on the important bits, and shown in a comment. It must be kept updated with any changes you put in.

Along with that, please have a description at the top of each test about what you're doing and expecting (similar to my notes in my review), so that we (and future developers) will know for sure what's happening in each test.

The docstrings themselves need to be shorter. The "case 1," "case 2," etc. won't make any sense without knowing about my review, and nobody should care down the road. So, the docstrings should be short, but self-describing, and should be about the function ultimately being tested. So, things like:

Testing GitClient.parse_revision_spec with target branch off a tracking branch aligned with the remote
Testing GitClient.parse_revision_spec with target branch off a tracking branch with changes since the remote
Testing GitClient.parse_revision_spec with target branch off a branch not properly tracking the remote

etc. So, similar to what I already have as the sections in my review, but written to make it clear that we're dealing with GitClient.parse_revision_spec.

Once that's all done, I'll have a better starting point for verifying that these are doing the right thing.

Thanks!

Change Summary:

Added documentation and visualization for test cases

Diff:

Revision 3 (+333 -6)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 3)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 3)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

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

Show all issues

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

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 3)
Show all issues
```
Col: 11
 W291 trailing whitespace
```
rbtools/clients/tests.py (Diff revision 3)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 3)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

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

Show all issues

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

Testing Done:

		The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.

		Update - I have now written tests for conditions 3 through 6 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.

	+	Update 2 - Documentation and visualization have been added to the tests to aid understanding. I hope this will help the mentors give me feedback.
	+
		Note - As of now, the test functions are poorly named (i.e. case one, case two etc.). This is just to signal to the mentors which conditions they're trying to test. I will improve the names once the content of the test cases is apporved by the mentors.

		I also had to alter one of the existing tests in order to align it with the new algorithm.

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

Show all issues

Since youre modifying the docstring for this function, could you reflow it into our document style.

e.g.

```
Single line summary

Multi-line description

Args:
arg_name (type of arg_name):
Description of arg name.

Returns:
return_type:
Description of what is returned.
"""

rbtools/clients/git.py (Diff revision 3)
Show all issues
```
Needs a docstirng.
```

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

Show all issues

Prefer '--remotes=%s' % remote. String interpolation is faster than concatenation in general.

rbtools/clients/git.py (Diff revision 3)
Show all issues
```
Missing a period.
"at a commit" ?
```
rbtools/clients/git.py (Diff revision 3)
Show all issues
```
Again prefer interpolation here.
```

Change Summary:

Fixed Issues Opened by Garret (i.e. docstrings and interpolation)

Diff:

Revision 4 (+367 -30)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 4)
Show all issues
```
Col: 28
 W291 trailing whitespace
```

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 4)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 4)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

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

Show all issues

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

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

Show all issues

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

Change Summary:

Added the last three unit tests along with documentation/visualization. The unit tests are now complete and waiting on review!

Diff:

Revision 5 (+555 -30)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 5)
Show all issues
```
Col: 28
 W291 trailing whitespace
```

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 5)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 5)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 5)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 5)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

rbtools/clients/git.py (Diff revision 5)
Show all issues
```
"Parse"
```
rbtools/clients/git.py (Diff revision 5)
Show all issues
```
"A dictionary with the following keys:
```

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

Show all issues

Can you reformat this to be a reStructuredText definition list, e.g.

```python
"""

A dictionary with the following keys:

``'base'`` (unicode):
    Description of 'base

``'tip'`` (unicode):
    Description of tip

``'commit_id'`` (unicode):
    Description of commit_id

"""

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

Show all issues

Docstrings should be in the following format:

"""Single line summary

Multi-line description
"""

We also want to use the imperitive mood ("Return") here.

This docstring will also need Args and Returns sections.

rbtools/clients/git.py (Diff revision 5)
Show all issues
```
Single quotes for strings.
```

Change Summary:

Fixed stylistic and documentation issues brought up by Barret, ReviewBot

Summary:

-	[WIP] Adding in a smarter parent-base finding algorithm
+	Adding in a smarter parent-base finding algorithm

Diff:

Revision 6 (+583 -32)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 6)
Show all issues
```
Col: 11
 W291 trailing whitespace
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

Change Summary:

Fixed stylistic (line length) issues brought up by ReviewBot

Diff:

Revision 7 (+593 -32)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py

Change Summary:

Updated "Testing Done" description

Testing Done:

~		The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.
	~	The algorithm has been tested extensively. More specifically, GitClient.parse_revision_spec has been tested on the conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). Detailed documentation and visualization have been added to the tests to aid understanding. I hope this will help a reader understand and review the tests better.

~		Update - I have now written tests for conditions 3 through 6 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.
	~	Specifically, the conditions being tested are -

~		Update 2 - Documentation and visualization have been added to the tests to aid understanding. I hope this will help the mentors give me feedback.
	~	0) Testing GitClient.parse_revision_spec with target branch off a tracking branch not aligned with the remote.
	+	1) Testing GitClient.parse_revision_spec with target branch off a tracking branch aligned with the remote.
	+	2) Testing GitClient.parse_revision_spec with target branch off a tracking branch with changes since the remote.
	+	3) Testing GitClient.parse_revision_spec with target branch off a branch not properly tracking the remote.
	+	4) Testing GitClient.parse_revision_spec with a target branch that merged a tracking branch off another tracking branch.
	+	5) Testing GitClient.parse_revision_spec with a target branch posted off a tracking branch that merged another tracking branch.
	+	6) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch without any tracking branches.
	+	7) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch that is aligned to the same commit as another remote branch.
	+	8) Testing GitClient.parse_revision_spec with a target branch not up-to-date with a remote branch.
	+	9) Testing GitClient.parse_revision_spec with a target branch that has branches from different remotes in its path.

~		Note - As of now, the test functions are poorly named (i.e. case one, case two etc.). This is just to signal to the mentors which conditions they're trying to test. I will improve the names once the content of the test cases is apporved by the mentors.
	~	Note - The above test descriptions make more sense when we actually view the commit graphs representing each of these scenarios. These commit graphs can be found as part of the documentation of my tests.
	+
	+	Note 2 - The test functions are named in a serial manner (i.e. with suffixes case_one, case_two ... case_nine etc.). This was done because it was not always easy to capture what the test was trying to test in a few words. Hence giving uniform but serial names helped because essentially, all of these tests test the same thing - i.e. whether GitClient.parse_revision_spec can find a good parent, parent_base in each of these scenarios.

		I also had to alter one of the existing tests in order to align it with the new algorithm.

Ship it!

I've been doing a bunch of testing on this, and after I make some style/documentation changes, I'm going to push this. Thanks for your work!

Status: Closed (submitted)

Change Summary:

Pushed to master (b8d1b82)