Summary

Update the dvcs branch for ES6

Review Request #8056 — Created March 12, 2016 and submitted March 15, 2016, 7:56 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

dvcs

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This patch updates the RB.DiffCommit model and
RB.DiffCommitIndexView view to use ES6. As well, the doc comments
have been updated to align with our style guide.

Testing Done

Manually tested that the diff commit index still worked.

Issues

Description	From	Last Updated
This should probably just use a defaults object instead of a method.	david	March 12, 2016, 5:05 p.m.
This will iterate over non-own properties too. How about: for (let property of Object.keys(this.attributes.lineCounts)) {	david	March 12, 2016, 5:09 p.m.
This can use the method() sugar.	david	March 12, 2016, 5:09 p.m.
How about: for (let [i, diffCommit] of Array.entries(this.collection.models)) {	david	March 12, 2016, 5:10 p.m.
Can you move this up above the "tr" definition? This seems like it doesn't fit in with the rest of …	david	March 12, 2016, 5:11 p.m.
Template string?	david	March 12, 2016, 5:11 p.m.
Template string?	david	March 12, 2016, 5:11 p.m.
const?	david	March 12, 2016, 5:11 p.m.
Can you align the - with "offset"?	david	March 12, 2016, 5:11 p.m.
Alignment?	david	March 12, 2016, 5:12 p.m.
This should be indented 2 more spaces.	david	March 14, 2016, 5:55 p.m.
Should be indented 2 more spaces.	david	March 14, 2016, 5:55 p.m.
Ooh, even better: return Object.values(this.get('lineCounts')) .reduce((a, b) => a + b);	david	March 14, 2016, 6:51 p.m.
return this.collection.any( model => model.get('historyEntrySymbol') !== ' ');	david	March 14, 2016, 6:53 p.m.
Can be const.	david	March 14, 2016, 6:54 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js (Diff revision 1)

The issue has been dropped. Show all issues

This should probably just use a defaults object instead of a method.

brennie

March 12, 2016, 5:06 p.m.

lineCounts is an object, so we should use a function here.

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

This will iterate over non-own properties too. How about:

for (let property of Object.keys(this.attributes.lineCounts)) {

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
This can use the method() sugar.
```

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

How about:

for (let [i, diffCommit] of Array.entries(this.collection.models)) {

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Can you move this up above the "tr" definition? This seems like it doesn't fit in with the rest of the code around it.

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Template string?
```
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Template string?
```
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
const?
```
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you align the - with "offset"?
```

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Alignment?

Change Summary:

Address David's issues

Diff:

Revision 2 (+553 -466)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
This should be indented 2 more spaces.
```

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revisions 1 - 2)

The issue has been resolved. Show all issues

Should be indented 2 more spaces.

Change Summary:

Adress David's issues

Diff:

Revision 3 (+553 -466)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js

A couple more suggestions to tighten things up:

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js (Diff revision 3)

The issue has been resolved. Show all issues

Ooh, even better:

return Object.values(this.get('lineCounts'))
    .reduce((a, b) => a + b);

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 3)

The issue has been resolved. Show all issues


return this.collection.any(
    model => model.get('historyEntrySymbol') !== ' ');

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Can be const.
```

Change Summary:

Address David's issues

Diff:

Revision 4 (+543 -466)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.es6.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to dvcs (94ca2eb)