Comment serialization cleanup part 7: Change diff comment format.

Review Request #13661 — Created March 21, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

The diff comment serialization worked very differently from file
attachment and screenshot comment serialization. This change updates
that to match.

The structure used to be:

[
    {
        "linenum": X,
        "num_lines": Y,
        "comments": [
            ... comments ...
        ]
    },
    ...
]

Now it's:

{
    "X-Y": [
        ... comments ...
    ]
}

The object keys in the new format are used to group all comments on the
same block together, but are otherwise opaque. Instead, like we do for
file attachments, we'll pick the line and num_lines values out of
the first comment in the list.

It's arguable that the diff format was better than the one used for file
attachments, since the line number and number of lines for each block
really should be serialized separately from the individual comments.
Unfortunately, the file attachment format isn't easily changeable because
extensions that add Review UIs exist in the wild, and we need to
maintain compatibility there.

  • Loaded a diff with existing comments and saw that they all appeared
    correctly.
  • Ran python tests.
  • Ran js-tests.
Summary ID
Comment serialization cleanup part 7: Change diff comment format.
The diff comment serialization worked very differently from file attachment and screenshot comment serialization. This change updates that to match. The structure used to be: ``` [ { "linenum": X, "num_lines": Y, "comments": [ ... comments ... ] }, ... ] ``` Now it's: ``` { "X-Y": [ ... comments ... ] } ``` The object keys in the new format are used to group all comments on the same block together, but are otherwise opaque. Instead, like we do for file attachments, we'll pick the `line` and `num_lines` values out of the first comment in the list. It's arguable that the diff format was better than the one used for file attachments, since the line number and number of lines for each block really should be serialized separately from the individual comments. Unfortunately, the file attachment format isn't easily changeable because extensions that add Review UIs exist in the wild, and we need to maintain compatibility there. Testing Done: - Loaded a diff with existing comments and saw that they all appeared correctly. - Ran python tests. - Ran js-tests.
7929db2ad40d313998b201468cfb0748f4c4a370
Description From Last Updated

undefined name 'DiffSet' Column: 14 Error code: F821

reviewbotreviewbot

dictionary key 'index' repeated with different values Column: 17 Error code: F601

reviewbotreviewbot

These don't support generics on Python 3.8.

chipx86chipx86

Version Added?

chipx86chipx86

This should use :file:.

chipx86chipx86

Version Added?

chipx86chipx86

These should be documented.

chipx86chipx86

This needs to be the full module path.

chipx86chipx86

These are missing docs.

chipx86chipx86

This isn't specific to this change, but while I'm here... I think the parent class should take generics for the …

chipx86chipx86

Looks like we're missing typing for this (unless it's in a base class?)

chipx86chipx86

Is this covered by the previous typing changes?

chipx86chipx86

Can we work toward keyword-only arguments for these constructors?

chipx86chipx86

Should DiffReviewUI just take these as keyword arguments?

chipx86chipx86

Should be :file:.

chipx86chipx86

Should probably be import type.

chipx86chipx86

Missing docs.

chipx86chipx86

Missing docs.

chipx86chipx86

Do we not get these types from the serialized data?

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
david
david
david
chipx86
  1. 
      
  2. reviewboard/reviews/ui/diff.py (Diff revision 5)
     
     

    Version Added?

  3. reviewboard/reviews/ui/diff.py (Diff revision 5)
     
     

    This should use :file:.

  4. reviewboard/reviews/ui/diff.py (Diff revision 5)
     
     

    Version Added?

  5. reviewboard/reviews/ui/diff.py (Diff revision 5)
     
     
     
     

    These should be documented.

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

    This needs to be the full module path.

  7. reviewboard/reviews/ui/diff.py (Diff revision 5)
     
     
     

    This isn't specific to this change, but while I'm here...

    I think the parent class should take generics for the comment type and serialized comment type. That way there's no need to cast everywhere, and callers can always get the right typing when requesting comment types, when accepting a type of the base class.

    1. Will play with that in a separate change.

  8. reviewboard/reviews/views/diffviewer.py (Diff revision 5)
     
     
     
     

    Is this covered by the previous typing changes?

    1. Those typing changes actually came after this in my stack. I'll double check that this gets undone there.

  9. reviewboard/reviews/views/diffviewer.py (Diff revision 5)
     
     

    Can we work toward keyword-only arguments for these constructors?

  10. reviewboard/reviews/views/diffviewer.py (Diff revision 5)
     
     
     
     

    Should DiffReviewUI just take these as keyword arguments?

  11. Should probably be import type.

  12. Do we not get these types from the serialized data?

    1. With all the indirection with this.commentBlockModel, it doesn't have a good idea of what parse() returns. This is in my notes to revisit later to try to improve.

  13. 
      
david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revisions 5 - 6)
     
     

    These don't support generics on Python 3.8.

  3. reviewboard/reviews/ui/diff.py (Diff revisions 5 - 6)
     
     
     
     
     
     

    These are missing docs.

  4. reviewboard/reviews/ui/diff.py (Diff revisions 5 - 6)
     
     

    Looks like we're missing typing for this (unless it's in a base class?)

    1. It's typed in the base ReviewUI class.

  5. 
      
david
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (126cb14)
Loading...