Introduce file attachment states.

Review Request #13238 — Created Aug. 27, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

Our file attachments can be in many different states, but we don't really have
a solid concept of these states in our codebase so far. We also don't have a
formal way of determining which state a file attachment is in.

This change introduces file attachment states and allows us to efficiently
get the state of a file attachment through its review request or draft.

This sets the foundation for an upcoming change that improves how we display
file attachments on a review request and how users can manage them.

  • Ran unit tests.
  • Ran JS unit tests.
  • Used in upcoming changes.
Summary ID
Introduce file attachment states.
f81a59d3ac1b258a3dffee7a378a3d0856a7a088
Description From Last Updated

Can we make a note to change this to use StrEnum once we're 3.11+?

daviddavid

I wonder if it's worth defining a TypeAlias for this (perhaps just alias for List[FileAttachment]). It feels pretty repetitive.

daviddavid

Can you add a module docstring while here?

chipx86chipx86

This should probably be a Sequence[FileAttachment], rather than a List[FileAttachment], and I'll go over why. It's a subtle thing, something …

chipx86chipx86

Can you add a module docstring?

chipx86chipx86

Are these meant to be used as labels, or IDs? "State" reads to me like an internal value, but the …

chipx86chipx86

We should designate all these keyword-only arguments.

chipx86chipx86

If the method is on the same class, you can leave off the class name.

chipx86chipx86

Each of these should be typed as Set[Any]. Note the Any is actually correct, as django-stubs types the primary key …

chipx86chipx86

The problem with the or here is that if the caller explicitly passes in an empty list, it'll cause a …

chipx86chipx86

I couldn't find the change.. Where does sort= come from?

chipx86chipx86

These should be typed.

chipx86chipx86

Missing a type.

chipx86chipx86

Missing a type.

chipx86chipx86

Should designate these keyword-only arguments.

chipx86chipx86

I'd say we probably should avoid the **kwargs for this case, since this is going to always want to exactly …

chipx86chipx86

For here and the reference below, the full module path to ReviewRequest must be specified. For this sort of case, …

chipx86chipx86

Missing , optional.

chipx86chipx86
david
  1. 
      
  2. Show all issues

    Can we make a note to change this to use StrEnum once we're 3.11+?

  3. reviewboard/reviews/models/review_request.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    I wonder if it's worth defining a TypeAlias for this (perhaps just alias for List[FileAttachment]). It feels pretty repetitive.

  4. 
      
maubin
chipx86
  1. This is great work.

    You're going to scroll down and see a giant comment. Don't worry, it's mostly a deep dive on invariance/covariance in typing. But it explains how List[FileAttachment] is, non-obviously, the wrong type to use for a list of file attachments in many cases.

    The rest are, for the most part, small doc-related things and some missing types.

  2. reviewboard/attachments/models.py (Diff revision 2)
     
     
    Show all issues

    Can you add a module docstring while here?

  3. Show all issues

    This should probably be a Sequence[FileAttachment], rather than a List[FileAttachment], and I'll go over why. It's a subtle thing, something I've been finding out the hard way about, and will be pointing out more in code reviews.

    List, Dict, Set, Tuple, etc. are very specific types that represent these mutable objects and behaviors. They're great for returning when we really want the caller to have all the same behaviors that the function itself has, such as adding/deleting items. But they're less ideal when we want to return results intended just for reading.

    Instead, we want to consider more general types, like Sequence, Mapping, and Iterator. These describe a subset of the behaviors, the read-only parts.

    There are a couple benefits to using more general types here:

    1. It conveys to the caller that the result is immutable/read-only.
    2. These general types are covariant, whereas the specific types are invariant, and these affect how the types can be used.

    Let's dig into what #2 means.

    Variance

    An invariant type is a specific type, and can only be assigned or passed in where that specific type is expected. A covariant type can be assigned that type or any subtype.

    This is easier with examples:

    class A:
        pass
    
    class B(A):
        pass
    
    class C(B):
        pass
    
    # Lists are invariant:
    l1: List[B] = [B(), C()]        # OK
    l2: List[B] = [A()]             # Not OK: Supertype assigned
    l3: List[A] = l1                # Not OK: List[A] and List[B] are invariant
    l4: List[Optional[B]] = l1      # Not OK: Same reason
    
    # Sequences are covariant:
    s1: Sequence[B] = [B(), C()]    # OK
    s2: Sequence[B] = [A()]         # Not OK: Supertype assigned
    s3: Sequence[A] = s1            # OK: Sequence[A] is covariant, allowing subtype of Sequence[B]
    s4: Sequence[Optional[B]] = s1  # OK: Same reason
    
    # Let's try mixing-and-matching:
    s5: Sequence[B] = l1            # OK: List[B] is a subtype of covariant Sequence[B]
    l5: List[B] = s1                # Not OK: Sequence[B] is not a subtype of List[B].
    

    (Note that the variance here is about the whole type, such as List[B], though that can be controlled if you were making your own Generics. Bigger topic.)

    There's also contra-variance, which is the opposite of covariance (they can use a type or any parent type).

    The reason for List being invariant is that l1 is a type that expects B or subclasses as items, any B or a subclass can be appended to it, and all read-related functions can therefore ensure a B or subclass. However, l3 expects A or subclasses, meaning a SubA could be appended in, which List[B] would not allow. That's why the types are not compatible in the l3 = l1 assignment above.

    However, s1 doesn't have this problem. There are no mutable operations exposed on a Sequence (or the other general types I listed), so the guarantee is always that items of the type are going to be returend. So we can do s3 = s1 without a problem. s3 says that all items are a A or subclass, and since we can't mutate it, and Sequence[B] is a B or subclass, that fulfills the contract.

    Same with Optional[...] in those types, because Optional[B] means B | None, which is a supertype of B, like A is a supertype of B.

    (Does that make sense? It might not at first. There's a lot to wrap one's head around, and that only gets worse once you dive in further.)

    Guidelines

    We can follow two simple rules to keep this relatively sane:

    1. Use the general types (Sequence, etc.) when returning or accepting values that are not/should not be modified.
    2. Use the specific types when building the values or returning/accepting values meant to be modified.

    In this case...

    Now, it probably doesn't make sense to make a TypeAlias for every possible value, but it very much depends. djblets.util.typing definitely has mutable and immutable versions of these types. But depending on context here, we may want a List[FileAttachment], a Sequence[FileAttachment], or an Iterator[FileAttachment], and maybe even more. And then there's Optional and friends.

    I know David mentioned that the Optional[List[FileAttachment]] was getting wordy, though, which is true. It's even more wordy with a Sequence. The problem in either case is that a TypeAlias here only saves two characters (especially if keeping the Optional[...]).

    In some cases, if the type is really module-specific, a private TypeAlias defined in TYPE_CHECKING would be the way to go, but too many modules need this.

    So it probably makes sense to define a FileAttachmentSequence and use that. If it ends up exceeding the line length limits, you can try:

    def foo(
       ...,
       inactive_file_attachments: Optional[
           FileAttachmentSequence
       ] = None,
       ...
    ) -> ...:
    

    Like that.

    Thanks for reading all this. Good copy/paste fodder that I should probably put in Notion.

    1. That does make sense. Thanks for taking the time to explain this to me, I appreciate it!

  4. Show all issues

    Can you add a module docstring?

  5. Show all issues

    Are these meant to be used as labels, or IDs? "State" reads to me like an internal value, but the contents read like human-readable.

    If it's internal state IDs, let's use slug format (lowercase, - instead of spaces).

    If it needs to be shown to users, we'd have to localize, so no enums. In this case, I'd still recommend we use slugs here, and then map that to localized strings elsewhere.

    1. I meant to use them as both internal state IDs and actual human-readable labels. But as you mentioned it'd be better to go the slug route so I'll go with that and map to localized strings somewhere in the frontend.

  6. Show all issues

    We should designate all these keyword-only arguments.

  7. Show all issues

    If the method is on the same class, you can leave off the class name.

  8. Show all issues

    Each of these should be typed as Set[Any].

    Note the Any is actually correct, as django-stubs types the primary key as an Any. This is because technically, it can be a string, integer, UUID, or something else model/backend-specific.

  9. Show all issues

    The problem with the or here is that if the caller explicitly passes in an empty list, it'll cause a query. So some more verbose code's going to ultimately be needed here.

    Same below.

  10. Show all issues

    I couldn't find the change.. Where does sort= come from?

    1. It's in /r/13237/ I wanted to avoid the queries for sorting the file attachments by display position since we don't really need them sorted here.

  11. Show all issues

    These should be typed.

  12. Show all issues

    Should designate these keyword-only arguments.

  13. Show all issues

    I'd say we probably should avoid the **kwargs for this case, since this is going to always want to exactly match the other method.

    (Alternatively we could just accept **kwargs and pass that in, but we'd lose all typing... Currently there's no way to say "the arguments of the function <blah>").

  14. Show all issues

    For here and the reference below, the full module path to ReviewRequest must be specified.

    For this sort of case, where you want the displayed text to show the ClassName.method format for a class in another module, you must do:

    :py:meth:`ReviewRequest.get_file_attachment_state()
    <reviewboard.reviews.models.review_request.ReviewRequest.get_file_attachment_state>`
    

    Note that:

    1. The () should be on there when explicitly naming this, to match the normal output of :py:meth:
    2. You can wrap at any . within the reference (custom extension of beanbag-docutils).
  15. reviewboard/reviews/models/review_request_draft.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
    Show all issues

    Missing , optional.

  16. 
      
maubin
chipx86
  1. 
      
  2. reviewboard/reviews/models/review_request.py (Diff revisions 2 - 3)
     
     
    Show all issues

    Missing a type.

    1. I define the variable and the type on line 1096.

  3. reviewboard/reviews/models/review_request.py (Diff revisions 2 - 3)
     
     
    Show all issues

    Missing a type.

    1. Same as above, I define the variable and the type on line 1097.

  4. 
      
david
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-6.x (6dc3520)