Summary

Refactor revision selectors for future file attachment support.

Review Request #6342 — Created Sept. 19, 2014 and submitted Sept. 20, 2014, 11:10 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

df43e22...

Reviewers

Groups

reviewboard

People

Description

This change refactors the DiffRevisionSelectorView control to abstract out
common functionality into a new RevisionSelectorView abstract base class. This
is then inherited by the existing DiffRevisionSelectorView and a new
FileAttachmentRevisionSelectorView.

Testing Done

Tested the various behavior in the DiffRevisionSelectorView.
FileAttachmentRevisionSelectorView is currently un-tested.

Issues

Description	From	Last Updated
Why aren't we using initialize? Might also be nice for future-expansion and consistency to have this take an options .	chipx86	Sept. 19, 2014, 11:03 a.m.
We should also assert that options isn't undefined.	chipx86	Sept. 20, 2014, 11:08 a.m.
I noticed the parameters are getting documented everywhere. How about having this take an options instead, so it's all self-documenting?	chipx86	Sept. 19, 2014, 11:04 a.m.
Should optimistically be listed alphabetically.	chipx86	Sept. 19, 2014, 11:06 a.m.

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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/views/revisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffCommentsHintView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/views/revisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffCommentsHintView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/static/rb/js/views/revisionSelectorView.js (Diff revision 1)

The issue has been resolved. Show all issues

Why aren't we using initialize?

Might also be nice for future-expansion and consistency to have this take an options .

reviewboard/static/rb/js/views/revisionSelectorView.js (Diff revision 1)

The issue has been dropped. Show all issues

I noticed the parameters are getting documented everywhere. How about having this take an options instead, so it's all self-documenting?

david Sept. 19, 2014, 11:06 a.m.

I've moved the initialActive parameter into the options for initialize, so this isn't confusing anymore.

reviewboard/staticbundles.py (Diff revision 1)

The issue has been resolved. Show all issues

Should optimistically be listed alphabetically.

david Sept. 19, 2014, 11:06 a.m.

I'm really not at all sure what "alphabetically" means for this list, but I've moved this to come above the various file attachment views, since fileAttachmentRevisionSelectorView.js will need to go in after this and before fileAttachmentReviewableView.js

Commit:

7cf184f6825195bedc5b2997bca81a765a9e5411

df43e22e3b7716ff5856f03e7ae53e9ab42976b9

Diff:

Revision 2 (+310 -211)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/views/diffCommentsHintView.js
	reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
	reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
	reviewboard/static/rb/js/views/revisionSelectorView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/views/revisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffCommentsHintView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/views/revisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffCommentsHintView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Ship it!

reviewboard/static/rb/js/views/revisionSelectorView.js (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
We should also assert that options isn't undefined.
```

Status:: Completed
Change Summary:: Pushed to master (470e7b8)