Implement a JSON Review UI that allow user to perform actions on json files

Review Request #9515 — Created Jan. 21, 2018 and updated

ridum
Review Board
release-4.0.x
reviewboard, students

Json files is handled as an uneditble attachments in the current
version.
I implemented a new class JSONReviewUI that
enhence Review board's ability to process JSON file.

New features include:

  • Rendering JSON in an easy-to-read format.
  • Allowing the user to comment on the prettified file based on
    line numbers.
  • Auto-sorting of output keys.
  • Allow user to compare the changes of files in diffs
  • A thumbnail that render in the easy-to-read format, too.

Ran unit tests.

Tested the development environment with:

  • Properly formatted JSON files.
  • JSON files with long lines.
  • Minified JSON files.
  • Highly-nested JSON files.
Loading file attachments...

  • 0
  • 0
  • 71
  • 0
  • 71
Description From Last Updated
brennie
  1. 
      
    1. What will be the best way to check the line has less than 72 characters?

  2. Can you wrap your description at 72 characters and format it as markdown?

    We can do stuff like:

    New features include:
    
     - Rendering JSON in an easy-to-read format.
     - Allowing the user to comment on the prettified file based on line numbers.
     - Auto-sorting of output keys.
    

    etc.

    Which will render as a list.

  3. As for your testing done, how about:

    Ran unit tests.
    Tested the development environment with:
    
     - Properly formatted JSON files.
     - JSON files with long lines.
     - Minified JSON files.
     - Highly-nested JSON files.
    

    The coverage percentage is not required.

  4. This needs some unit tests ensuring that (at a minimum):

    • the JSON UI is used for JSON files
    • prettified JSON files are ordered consistently between viewings
    • output is actually prettified
    1. I could not come up with a good idea to test the prettified JSON , so I uploaded an ugly unsorted JSON and a sorted,prettified JSON and check their equallity after the render;

      I test tthe JSON UI in attachment files instead of ui files.

      The Markdown does not have basic unit test.

      For the Json document that has incorrect format, Reviewboard will currently render "Error while rendering Markdown content:"
      I am not sure if theis is the correct behavior.

  5. reviewboard/reviews/ui/jsonui.py (Diff revision 1)
     
     

    How about JSONReviewUI ? It's a little shorter and gets the same point across.

  6. reviewboard/reviews/ui/jsonui.py (Diff revision 1)
     
     

    All methods should have docstring documenting what they do. Check out this guide for our docstring requirements.

  7. 
      
  1. 
      
  2. reviewboard/reviews/ui/jsonui.py (Diff revision 2)
     
     

    Do you need to explicitly open the file when use already use the with keyword? or do something like
    with open(self.obj.file) as f:

  3. reviewboard/reviews/ui/jsonui.py (Diff revision 2)
     
     

    Is the indent always going to be 4? Maybe keep as a constant?

    1. In my opinion a constant should only be introduced if it is going to be used it other place . For this particular case. The intend is only going to be used in here, so I keep it as a magic number.
      The indent hsould always be one tab which is 4 space

  4. reviewboard/reviews/ui/jsonui.py (Diff revision 2)
     
     

    Just out of curiosity, what does the _ do before the string?

    1. at line6 "from django.utils.translation import ugettext as _"
      So this is basically function call to render the string.
      for more, you can refer to https://docs.djangoproject.com/en/2.0/ref/utils/

  5. reviewboard/reviews/ui/tests.py (Diff revision 2)
     
     

    Typo: hekper -> helper

  6. reviewboard/reviews/ui/tests.py (Diff revision 2)
     
     

    Not sure if this will be a problem - but does os.path.join correctly change slashes based on the appropriate os?

    1. As I tested so far yes -- but I am gonna change the testing method

  7. reviewboard/reviews/ui/tests.py (Diff revision 2)
     
     

    Keep the function names consistent, so: test_wrong_formatted_JSON

  8. reviewboard/reviews/ui/tests.py (Diff revision 2)
     
     

    inconsistent function naming: test_prettified_JSON

  9. 
      
david
  1. 
      
  2. reviewboard/attachments/mimetypes.py (Diff revision 3)
     
     

    Instead of adding this to the text mimetype, I think we need a new JSONMimetype class that can run the source through DjbletsJSONEncoder like the JSONReviewUI does.

  3. reviewboard/reviews/ui/jsonui.py (Diff revision 3)
     
     

    Can you add a module docstring at the top of this file?

  4. reviewboard/reviews/ui/jsonui.py (Diff revision 3)
     
     

    This is kind of a weird docstring, and doesn't explain what the return (yield) type is. How about:

    """Render the text.
    
    This will render the JSON file in a more readable manner,
    reindenting the output, sorting keys within dictionaries,
    and applying syntax highlighting.
    
    Yields:
        unicode:
        Each line of the rendered output.
    """
    
  5. reviewboard/reviews/ui/markdownui.py (Diff revision 3)
     
     

    Let's just revert the changes to this file. We should clean up docstrings here (eventually), but it shouldn't be part of this change.

  6. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    Standard library imports should go in a section above 3rd-party imports.

  7. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     
     
     

    This docstring isn't in the proper format. How about just """Set up state for ImageReviewUI tests."""

  8. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    This should probably be called JSONReviewUITests to match the naming elsewhere.

  9. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    No blank line here.

  10. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     
     

    Blank line between these. This should also say "JSONReviewUI" to match the name of the class.

  11. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    Docstring summaries need to start with capital letters and end in periods. This also needs Args and Returns sections to explain what file_directory is and what the return value is.

    I think file_directory should also be called either filename or json_file. Because it's not a directory at all.

  12. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     
     
     

    We should put test data in a directory called "testdata", not "testJsonData".

    Right now we also have a weird mix of using os.path.join and string concatenation. Everything should be passed in to join and we shouldn't have any directory separators:

    file_path = os.path.join(
        os.path.dirname(__file__), 'testdata', filename)
    
  13. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    Blank line above this.

  14. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    file is a reserved word in Python. Let's change this name.

  15. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     
     
     

    It's kind of weird to have this set self.attachment as a side effect and then return the uploaded file object (which nothing uses). Let's change this to return the attachment instead of storing it in self.

  16. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    There's an extra space at the end of this docstring. Test docstrings should also always start with "Testing" and include full context. How about:

    Testing JSONReviewUI rendering nested objects

  17. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    Just initializing the review UI won't actually run the render function. I think you forgot the code to test get_rendered_lines()

  18. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     
     

    Same comment about docstrings. How about:

    Testing JSONReviewUI render with invalid JSON

  19. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    Can we rename all of these test files to be lower-case with hyphens instead of camelCase or CamelCase?

  20. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     

    u prefixes on strings aren't necessary since we import unicode_literals. You also don't need to use + to concatenate multiple lines (python will concatenate adjacent strings, like C).

  21. reviewboard/reviews/ui/tests.py (Diff revision 3)
     
     
     

    Format is wrong here and there's some spelling errors. How about:

    Testing JSONReviewUI indenting and sorting

  22. 
      
Review request changed
david
  1. 
      
  2. reviewboard/attachments/__init__.py (Diff revision 5)
     
     
     

    Undo this. ) should be on the previous line.

  3. reviewboard/attachments/mimetypes.py (Diff revision 5)
     
     
     

    These should be grouped with the other 3rd-party imports (back where they were)

    1. I am not sure what do you mean by grouped with the other 3rd-party imports; I have troubled figureing out what is the other 3rd party imports and how should I grouped them; I put the JSON Lexer in it, though.

    2. Python imports are in three groups:

      Standard library
      Third-party
      This module (aka reviewboard)

      You moved docutils.core to the 3rd-party group, correctly, but then moved these up into their own group. So it should look like:

      ... more stuff ...
      from djblets.util.filesystem import is_exe_in_path
      from djblets.util.templatetags.djblets_images import thumbnail
      from pygments import highlight
      from pygments.lexers import ClassNotFound, TextLexer, guess_lexer_for_filename
      
      from reviewboard.reviews.markdown_utils import render_markdown
      
  4. reviewboard/attachments/mimetypes.py (Diff revision 5)
     
     
     
     
     
     

    Since we're inheriting from TextMimetype we don't need to redefine these.

  5. reviewboard/attachments/mimetypes.py (Diff revision 5)
     
     

    typo: beautifed -> beautified

  6. reviewboard/attachments/tests.py (Diff revision 5)
     
     

    TextMimetype isn't used

  7. reviewboard/attachments/tests.py (Diff revision 5)
     
     

    Can you keep these names in alphabetical order?

  8. reviewboard/attachments/tests.py (Diff revision 5)
     
     

    Json -> JSON

  9. reviewboard/attachments/tests.py (Diff revision 5)
     
     
     

    This should fit on one line.

  10. reviewboard/reviews/ui/__init__.py (Diff revision 5)
     
     
     
     
     
     

    Keep in alphabetical order

  11. reviewboard/reviews/ui/__init__.py (Diff revision 5)
     
     

    Keep in alphabetical order.

  12. reviewboard/reviews/ui/jsonui.py (Diff revision 5)
     
     

    Instead of using logging.error with exc_info=True, you can use logging.exception.

  13. reviewboard/reviews/ui/markdownui.py (Diff revision 5)
     
     

    Docstring isn't in the proper format. Let's just avoid making any changes to this file--if we want to clean up code, it should be in separate changes.

  14. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    Test docstrings shouldn't end in periods (since the test runner will add "...")

  15. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    Test docstrings shouldn't end in periods (since the test runner will add "...")

  16. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    Since this is an internal helper, let's add a leading underscore: _get_json_file

  17. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    This needs to be the fully-qualified module path of the type: reviewboard.attachments.models.FileAttachment

  18. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    The directory should be called testdata, not testData.

  19. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    You're still storing this in self.attachment and then calling this method as if it doesn't have a return value. This should just be:

    return form.create()
    

    and then below, in the test methods:

    ui = JSONReviewUI(self.review_request,
                      self.get_json_file('filename.json'))
    
  20. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    Test docstrings shouldn't end in periods (since the test runner will add "...")

  21. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    No blank line here.

  22. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    Don't we want to assert something in here?

    1. I am not sure how to test the format here, This test is just making sure no exception are thrown. Formatting are tested by acturally interacting with the UI

    2. Assert that the result of this is equal to something, like you do for all the other tests.

  23. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    Test docstrings shouldn't end in periods (since the test runner will add "...")

  24. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    No blank line here.

  25. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    Test docstrings shouldn't end in periods (since the test runner will add "...")

  26. reviewboard/reviews/ui/tests.py (Diff revision 5)
     
     

    No blank line here.

  27. 
      
david
  1. 
      
  2. reviewboard/attachments/mimetypes.py (Diff revision 6)
     
     
     

    We should still wrap by putting everything in parens instead of using the continuation character.

  3. reviewboard/reviews/ui/jsonui.py (Diff revision 6)
     
     

    This should probably be the same as the class docstring ("A review UI for JSON files")

  4. reviewboard/reviews/ui/jsonui.py (Diff revision 6)
     
     

    "Review UI" -> "review UI"

  5. reviewboard/reviews/ui/jsonui.py (Diff revision 6)
     
     
     

    The line break here is ugly. Please combine these.

  6. reviewboard/reviews/ui/tests.py (Diff revision 6)
     
     

    Blank line isn't necessary here.

  7. reviewboard/reviews/ui/tests.py (Diff revision 6)
     
     

    Blank line isn't necessary here.

  8. reviewboard/reviews/ui/tests.py (Diff revision 6)
     
     

    json file -> JSON file

  9. reviewboard/reviews/ui/tests.py (Diff revision 6)
     
     

    Should list a type in parens:

    filename (unicode):
    
  10. reviewboard/reviews/ui/tests.py (Diff revision 6)
     
     

    Indent this 4 more spaces.

  11. reviewboard/reviews/ui/tests.py (Diff revision 6)
     
     

    This is an extra call to get_rendered_lines which is not needed.

  12. reviewboard/reviews/ui/tests.py (Diff revision 6)
     
     

    It might be nicer to hard-code the "correct" result rather than running it through the review UI. Right now if there's some change or bug that affects highlighting or reindenting, it will affect both the original and the "correct" file, making it possibly slip through the test.

    1. I think this is what we want; the change or bug in highlighting or reindenting will not affect the JSONUI tests.The test of JSON UI should simply focus on checking if the JSON file can be sorted correctly.

      I actually feel like there is actually not need a "correct" result for this project. Because I think everything that satisfied what user want can be considered "correct". By my view, usability testings are sufficient and are better since the user may wanna change their requirement at any moment.

      On the other hand, the dard-code "correct" answer will be a "magic" string of HTML file. This make the debugging of the tests very difficult

  13. reviewboard/reviews/ui/tests.py (Diff revision 6)
     
     
     

    We wrap long strings with the space characters and the end of previous lines instead of the start of the next.

  14. reviewboard/reviews/ui/tests.py (Diff revision 6)
     
     

    Same here--let's hard-code the correct output instead of running it through the review UI.

    1. same reason as above.

  15. 
      
Review request changed
Loading...