Summary

File modification checking multi-line support and cleanup

Review Request #10280 — Created Oct. 25, 2018 and submitted Nov. 30, 2018, 11:26 a.m.

Information

Owner

alextechcc

Repository

ReviewBot

Branch

master

Bugs

Depends On

Blocks

10195

Commit

502f6f1...

Reviewers

Groups

reviewbot, students

People

Description

Multiple TODOs have been addressed in _is_modified, so that tools
commenting on multiple lines have modifications properly checked. The
algorithm has been made more efficient, only checking relevant
(modified) chunks to see if they are in the range of modification.

A patch was also made to tools that 'globally comment' (I.E. make a
comment with a line of <=0). Previously, all this would do is round the
line number up to 1, (as the front-end does not support rendering
first_line=None), which had the unintended side-effect of tools
configured to only comment on modified code who make global comments, to
only have their comment go through if line 1 was modified. The
first_line=1 workaround is still used, and the file is assumed to be
modified.

Testing Done

I've tested _is_modified through my nyc plugin directly, and
tested File.comment() manually through a stub-plugin. Waiting on test
suite creation to mock up standalone tests.

Issues

Description	From	Last Updated
"ReviewBoard" -> "Review Board"	david	Oct. 30, 2018, 5:17 p.m.
We generally avoid Python's ternary form because it's confusing and ugly.	david	Oct. 30, 2018, 5:17 p.m.
Can we have one blank line above this?	david	Oct. 30, 2018, 5:17 p.m.
This comment doesn't really make sense.	david	Nov. 22, 2018, 9:33 p.m.
``None``	brennie	Nov. 26, 2018, 3:41 p.m.

flake8 passed.

JSHint passed.

bot/reviewbot/processing/review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"ReviewBoard" -> "Review Board"
```
bot/reviewbot/processing/review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We generally avoid Python's ternary form because it's confusing and ugly.
```
bot/reviewbot/processing/review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we have one blank line above this?
```

Change Summary:

Incorporated formatting and style changes.

Commit:

53b8b990530d51ae8c0150efdbf91e3451d21fdd

2efa09d38cb47ee4029e6846b6eff887e45d7ce5

Diff:

Revision 2 (+32 -17)

Show changes

bot/reviewbot/processing/review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Accept negative line numbers also as global comments, assume files are modified if they're in the file diff.

Description:

		Multiple TODOs have been addressed in _is_modified, so that tools
		commenting on multiple lines have modifications properly checked. The
		algorithm has been made more efficient, only checking relevant
		(modified) chunks to see if they are in the range of modification.

		A patch was also made to tools that 'globally comment' (I.E. make a
~		comment with a line of 0). Previously, all this would do is round the
	~	comment with a line of <=0). Previously, all this would do is round the
		line number up to 1, (as the front-end does not support rendering
		first_line=None), which had the unintended side-effect of tools
~		configured to only comment on modified code who make global comments,
~		to only have their comment go through if line 1 was modified. The
~		first_line=1 workaround is still used, but the modification check now
~		checks the whole file for modifications.
	~	configured to only comment on modified code who make global comments, to
	~	only have their comment go through if line 1 was modified. The
	~	first_line=1 workaround is still used, and the file is assumed to be
	~	modified.