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

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

Information

Review Board
release-4.0.x

Reviewers

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.

Description From Last Updated

Can you wrap your description at 72 characters and format it as markdown? We can do stuff like: New features …

brenniebrennie

As for your testing done, how about: Ran unit tests. Tested the development environment with: - Properly formatted JSON files. …

brenniebrennie

This needs some unit tests ensuring that (at a minimum): the JSON UI is used for JSON files prettified JSON …

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Do you need to explicitly open the file when use already use the with keyword? or do something like with …

FL floriecai

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

FL floriecai

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

FL floriecai

Typo: hekper -> helper

FL floriecai

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

FL floriecai

Keep the function names consistent, so: test_wrong_formatted_JSON

FL floriecai

inconsistent function naming: test_prettified_JSON

FL floriecai

Instead of adding this to the text mimetype, I think we need a new JSONMimetype class that can run the …

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

No blank line here.

daviddavid

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

daviddavid

Docstring summaries need to start with capital letters and end in periods. This also needs Args and Returns sections to …

daviddavid

We should put test data in a directory called "testdata", not "testJsonData". Right now we also have a weird mix …

daviddavid

Blank line above this.

daviddavid

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

daviddavid

It's kind of weird to have this set self.attachment as a side effect and then return the uploaded file object …

daviddavid

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

daviddavid

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

daviddavid

Same comment about docstrings. How about: Testing JSONReviewUI render with invalid JSON

daviddavid

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

daviddavid

u prefixes on strings aren't necessary since we import unicode_literals. You also don't need to use + to concatenate multiple …

daviddavid

Format is wrong here and there's some spelling errors. How about: Testing JSONReviewUI indenting and sorting

daviddavid

F401 'django.utils.translation.ugettext as _' imported but unused

reviewbotreviewbot

Undo this. ) should be on the previous line.

daviddavid

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

daviddavid

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

daviddavid

typo: beautifed -> beautified

daviddavid

TextMimetype isn't used

daviddavid

Can you keep these names in alphabetical order?

daviddavid

Json -> JSON

daviddavid

This should fit on one line.

daviddavid

Keep in alphabetical order

daviddavid

Keep in alphabetical order.

daviddavid

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

daviddavid

Docstring isn't in the proper format. Let's just avoid making any changes to this file--if we want to clean up …

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

The directory should be called testdata, not testData.

daviddavid

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

daviddavid

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

daviddavid

No blank line here.

daviddavid

Don't we want to assert something in here?

daviddavid

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

daviddavid

No blank line here.

daviddavid

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

daviddavid

No blank line here.

daviddavid

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

daviddavid

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

daviddavid

"Review UI" -> "review UI"

daviddavid

The line break here is ugly. Please combine these.

daviddavid

Blank line isn't necessary here.

daviddavid

Blank line isn't necessary here.

daviddavid

json file -> JSON file

daviddavid

Should list a type in parens: filename (unicode):

daviddavid

Indent this 4 more spaces.

daviddavid

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

daviddavid

It might be nicer to hard-code the "correct" result rather than running it through the review UI. Right now if …

daviddavid

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

daviddavid

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

daviddavid
RI
RI
brennie
  1. 
      
    1. What will be the best way to check the line has less than 72 characters?

  2. Show all issues

    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. Show all issues

    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. Show all issues

    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)
     
     
    Show all issues

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

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

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

  7. 
      
RI
RI
RI
FL
  1. 
      
  2. reviewboard/reviews/ui/jsonui.py (Diff revision 2)
     
     
    Show all issues

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

    Typo: hekper -> helper

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

    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)
     
     
    Show all issues

    Keep the function names consistent, so: test_wrong_formatted_JSON

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

    inconsistent function naming: test_prettified_JSON

  9. 
      
RI
david
  1. 
      
  2. reviewboard/attachments/mimetypes.py (Diff revision 3)
     
     
    Show all issues

    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)
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

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

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

    No blank line here.

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

    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)
     
     
    Show all issues

    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)
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    Blank line above this.

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

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

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

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

    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)
     
     
     
    Show all issues

    Same comment about docstrings. How about:

    Testing JSONReviewUI render with invalid JSON

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

    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)
     
     
    Show all issues

    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)
     
     
     
    Show all issues

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

    Testing JSONReviewUI indenting and sorting

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

    Undo this. ) should be on the previous line.

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

    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)
     
     
     
     
     
     
    Show all issues

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

  5. reviewboard/attachments/mimetypes.py (Diff revision 5)
     
     
    Show all issues

    typo: beautifed -> beautified

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

    TextMimetype isn't used

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

    Can you keep these names in alphabetical order?

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

    Json -> JSON

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

    This should fit on one line.

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

    Keep in alphabetical order

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

    Keep in alphabetical order.

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

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

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

    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)
     
     
    Show all issues

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

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

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

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

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

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

    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)
     
     
    Show all issues

    The directory should be called testdata, not testData.

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

    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)
     
     
    Show all issues

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

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

    No blank line here.

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

    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)
     
     
    Show all issues

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

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

    No blank line here.

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

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

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

    No blank line here.

  27. 
      
RI
david
  1. 
      
  2. reviewboard/attachments/mimetypes.py (Diff revision 6)
     
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

    "Review UI" -> "review UI"

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

    The line break here is ugly. Please combine these.

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

    Blank line isn't necessary here.

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

    Blank line isn't necessary here.

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

    json file -> JSON file

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

    Should list a type in parens:

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

    Indent this 4 more spaces.

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

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

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

    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)
     
     
     
    Show all issues

    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)
     
     
    Show all issues

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

    1. same reason as above.

  15. 
      
RI
Review request changed