Change Summary:
+ Screenshot
Added Files: |
---|
Review Request #3564 — Created Nov. 26, 2012 and submitted
A ReviewBoard Extension that demonstrates how to: - Register a ReviewUI for XML file attachments. - Register a Mimetype Handler to create XML thumbnails for these attachments. Currently: - Both the ReviewUI / Mimetype Handler provide syntax / keyword highlight for XML file via pygments.
Manually tested with ReviewUI integration through Extensions patch: http://reviews.reviewboard.org/r/3565/ - Successfully installed this extension, its ReviewUI, and its Mimetype Handler - Once installed, "Add Comment" button on XML attachments change to "Review" buttons. - Once installed, able to review XML file attachments via the "Review" button. - Once installed, XML file attachments now have thumbnails (if it was not enabled before) - Color markup for the XML file attachment correctly rendered through pygments.
Description | From | Last Updated |
---|---|---|
Should go alphabetical: pygments.formatters then pygments.lexers |
KA Karl-L | |
Any potential for IOError (e.g. file doesn't exist)? This might be handled elsewhere, so feel free to ignore if this … |
KA Karl-L | |
Two spaces between the imports and functions, one space between the functions. Also, not sure if there's any precedent (or … |
KA Karl-L | |
This should go right after "import logging" |
AA aam1r | |
Add a blank line between these two lines |
AA aam1r | |
Add space between these two lines |
AA aam1r | |
This looks a bit long for one line. Try moving the "% PACKAGE" To the next. |
|
|
Two blank lines. |
|
|
I don't think we need to subclass everything. What I'd prefer is to just instantiate a URLHook and pass it … |
|
|
Indentation looks off here. |
|
|
Same comment. |
|
|
I think it will read a little easier if you put "extension" on the next line. Also indent only 4 … |
|
|
Same. |
|
|
Should inherit from FileAttachmentReviewable. Those defaults are available in that parent class. The only one you introduce is 'rendered', which … |
|
|
I don't really know why we have this. I'd say unless there's an immediate reason for some dashboard, remove it. |
|
|
Add two blank lines above each function. |
|
|
This file should be called xml_review_ui.py |
|
|
Is this line too long? |
|
|
This file should be mimetypes.py |
|
|
This file should be xmlReviewableView.js |
|
xml_review_ui_extension/xml_review_ui_extension/XMLReviewUI.py (Diff revision 1) |
---|
Should go alphabetical: pygments.formatters then pygments.lexers
xml_review_ui_extension/xml_review_ui_extension/XMLReviewUI.py (Diff revision 1) |
---|
Any potential for IOError (e.g. file doesn't exist)? This might be handled elsewhere, so feel free to ignore if this is the case.
xml_review_ui_extension/xml_review_ui_extension/views.py (Diff revision 1) |
---|
Two spaces between the imports and functions, one space between the functions. Also, not sure if there's any precedent (or desire to go this way), but Django's class-based generic views would let you add these views inline in your urls.py modules. E.g. in urls.py, the url would look like: url(r'^$', TemplateView.as_view(template_name="xml_review_ui_extension/configure.html") along with the appropriate import. See https://docs.djangoproject.com/en/1.4/topics/class-based-views/ for more on class-based views. The project mentors may not be into using them, so if you're interested, double-check before going down that road.
Fixed imports and added error handling as per Karl's comments Also added force_unicdoe on data_string the read XML file
xml_review_ui_extension/xml_review_ui_extension/XMLReviewUI.py (Diff revision 2) |
---|
I think this will all fit in one line
xml_review_ui_extension/xml_review_ui_extension/XMLReviewUI.py (Diff revision 2) |
---|
This should go right after "import logging"
xml_review_ui_extension/xml_review_ui_extension/extension.py (Diff revision 2) |
---|
Add a blank line between these two lines
xml_review_ui_extension/xml_review_ui_extension/extension.py (Diff revision 2) |
---|
Add space between these two lines
Fixed minor style issue as per aam1r's comment.
Diff: |
Revision 3 (+191)
|
---|
Updated this extension to adhere to the changes made to extensions framework (See http://reviews.reviewboard.org/r/3578/)
Diff: |
Revision 4 (+191)
|
---|
Removed {{MEDIA_SERIAL}} as it no longer exists
Diff: |
Revision 5 (+192)
|
---|
What configuration are you planning on adding?
xml_review_ui_extension/xml_review_ui_extension/XMLReviewUI.py (Diff revision 5) |
---|
Because 1.7 requires Python 2.5 and newer, we can now use the "with" structure here to make this nicer.
xml_review_ui_extension/xml_review_ui_extension/extension.py (Diff revision 5) |
---|
Indentation looks off here.
xml_review_ui_extension/xml_review_ui_extension/extension.py (Diff revision 5) |
---|
I think it will read a little easier if you put "extension" on the next line. Also indent only 4 spaces from the "super"
xml_review_ui_extension/xml_review_ui_extension/views.py (Diff revision 5) |
---|
Add two blank lines above each function.
xml_review_ui_extension/setup.py (Diff revision 5) |
---|
This looks a bit long for one line. Try moving the "% PACKAGE" To the next.
xml_review_ui_extension/xml_review_ui_extension/extension.py (Diff revision 5) |
---|
I don't think we need to subclass everything. What I'd prefer is to just instantiate a URLHook and pass it the patterns. Keeps things simpler.
Should inherit from FileAttachmentReviewable. Those defaults are available in that parent class. The only one you introduce is 'rendered', which should be the only entry in here.
xml_review_ui_extension/xml_review_ui_extension/urls.py (Diff revision 5) |
---|
I don't really know why we have this. I'd say unless there's an immediate reason for some dashboard, remove it.
Trimmed XML Review UI Extension to retain only the minimally required content logic / files.
Testing Done: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+121)
|
Added XMLMimetype Handler to let this Extension also demonstrate registration of the XML Thumbnails feature through hooks.
Description: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||
Diff: |
Revision 7 (+141)
|
Fixed minor style issues
Diff: |
Revision 8 (+141)
|
---|
This is looking pretty good. My major comments are about the naming of the files, which doesn't match our conventions.
xml_review_ui_extension/xml_review_ui_extension/XMLReviewUI.py (Diff revision 8) |
---|
This file should be called xml_review_ui.py
xml_review_ui_extension/xml_review_ui_extension/XMLReviewUI.py (Diff revision 8) |
---|
Is this line too long?
xml_review_ui_extension/xml_review_ui_extension/XMLThumbnail.py (Diff revision 8) |
---|
This file should be mimetypes.py
Fixed various naming and syntax issues. Waiting on pending review-request for Text-based commenting to land to introduce it to this extension
Diff: |
Revision 9 (+141)
|
---|