Summary

Correctly return line counts in the FileDiffCollectionMixin

Review Request #10099 — Created July 24, 2018 and submitted Aug. 17, 2018, 2:54 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Blocks

10094

Commit

b2f5893...

Reviewers

Groups

reviewboard

People

Description

The FileDiffCollectionMixin incorrectly assumed that counts would
always be integers. However, before chunks are generated, some line
counts may be None. This patch works around that limitation and
returns the correct result for that case. Unit tests have been added to
test this behaviour.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
E501 line too long (81 > 79 characters)	reviewbot	July 24, 2018, 3:46 p.m.
F841 local variable 'filediffs' is assigned to but never used	reviewbot	July 24, 2018, 3:46 p.m.
This would be nicer as: if value: if counts[key] is None: counts[key] = value else: counts[key] += value	chipx86	July 31, 2018, 2:15 p.m.
Missing a test docstring.	chipx86	July 31, 2018, 2:15 p.m.
Is this the start of a brand new set of tests? If so, it should be its own test.	chipx86	July 31, 2018, 2:19 p.m.
F821 undefined name 'commits'	reviewbot	July 31, 2018, 2:24 p.m.
F821 undefined name 'commits'	reviewbot	July 31, 2018, 2:24 p.m.
F821 undefined name 'diffset'	reviewbot	July 31, 2018, 2:24 p.m.
F821 undefined name 'commits'	reviewbot	July 31, 2018, 2:24 p.m.
F821 undefined name 'commits'	reviewbot	July 31, 2018, 2:24 p.m.
E303 too many blank lines (2)	reviewbot	July 31, 2018, 2:24 p.m.
F821 undefined name 'diffset'	reviewbot	July 31, 2018, 2:24 p.m.
F821 undefined name 'diffset'	reviewbot	July 31, 2018, 2:24 p.m.
F821 undefined name 'commits'	reviewbot	July 31, 2018, 2:24 p.m.
F821 undefined name 'commits'	reviewbot	July 31, 2018, 2:24 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'filediffs' is assigned to but never used
```

Commit:

5caef7e863445d1918e894b193e48cded60ab1e2

35647a0eb4bd6008cd6ddd7c440847f04f1d75b6

Diff:

Revision 2 (+128 -2)

Show changes

	reviewboard/diffviewer/models/mixins.py
	reviewboard/diffviewer/tests/test_models_mixins.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/models/mixins.py (Diff revision 2)

The issue has been resolved. Show all issues

This would be nicer as:

if value:
    if counts[key] is None:
        counts[key] = value
    else:
        counts[key] += value

reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing a test docstring.
```

reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 2)

The issue has been resolved. Show all issues

Is this the start of a brand new set of tests? If so, it should be its own test.

Change Summary:

Addressed feedback.

Commit:

35647a0eb4bd6008cd6ddd7c440847f04f1d75b6

fb4a24af3a0d32fe4e27cca790011e21b8e46756

Diff:

Revision 3 (+136 -2)

Show changes

	reviewboard/diffviewer/models/mixins.py
	reviewboard/diffviewer/tests/test_models_mixins.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F821 undefined name 'commits'
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F821 undefined name 'commits'
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F821 undefined name 'diffset'
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F821 undefined name 'commits'
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F821 undefined name 'commits'
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F821 undefined name 'diffset'
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F821 undefined name 'diffset'
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F821 undefined name 'commits'
```
reviewboard/diffviewer/tests/test_models_mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F821 undefined name 'commits'
```

Commit:

fb4a24af3a0d32fe4e27cca790011e21b8e46756

b2f5893ccbe157c8c8317204b8998dd86d33f793

Diff:

Revision 4 (+135 -2)

Show changes

	reviewboard/diffviewer/models/mixins.py
	reviewboard/diffviewer/tests/test_models_mixins.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-4.0.x (27146e0)