Add typing and fix issues throughout the datagrids code.

Review Request #13355 — Created Oct. 17, 2023 and submitted

Information

Djblets
release-3.x

Reviewers

The datagrids code was old and crufty, and made some bad typing
assumptions all throughout. A few of these have bit us in the past, when
search indexing bots threw garbage at our datagrids and caused HTTP 500
errors. The mess of types have made it hard to reliably work on this
code, and since it needs some upcoming work, it was time to fix that up.

This change adds typing and documentation all throughout the datagrids
code. In the process, bad typing assumptions and scope errors were
fixed.

There's a lot of old, bad design patterns in this code, but this doesn't
try to change any of those. It's just building enough of a foundation to
start iterating on.

Djblets and Review Board unit tests pass.

Tested all of the datagrids used in Review Board.

Summary ID
Add typing and fix issues throughout the datagrids code.
The datagrids code was old and crufty, and made some bad typing assumptions all throughout. A few of these have bit us in the past, when search indexing bots threw garbage at our datagrids and caused HTTP 500 errors. The mess of types have made it hard to reliably work on this code, and since it needs some upcoming work, it was time to fix that up. This change adds typing and documentation all throughout the datagrids code. In the process, bad typing assumptions and scope errors were fixed. There's a lot of old, bad design patterns in this code, but this doesn't try to change any of those. It's just building enough of a foundation to start iterating on.
7f8b1c19d31e94db800032dfa3629404d2edb10e
Description From Last Updated

Hold off on this. Some corner cases to resolve.

chipx86chipx86

Missing the type.

maubinmaubin

Missing the type.

maubinmaubin

While you're here could you fix this typo: "conver" -> "convert".

maubinmaubin

We're not doing anything with timezone in this function. I think we're missing a self.timezone = timezone?

maubinmaubin

I think the type in the docs is supposed to be dict or or django.template.context.Context.

maubinmaubin

Missing documentation for *args and **kwargs.

maubinmaubin
chipx86
  1. 
      
  2. Show all issues

    Hold off on this. Some corner cases to resolve.

  3. 
      
chipx86
maubin
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 2)
     
     
    Show all issues

    Missing the type.

  3. djblets/datagrid/grids.py (Diff revision 2)
     
     
    Show all issues

    Missing the type.

  4. djblets/datagrid/grids.py (Diff revision 2)
     
     
    Show all issues

    While you're here could you fix this typo: "conver" -> "convert".

  5. djblets/datagrid/grids.py (Diff revision 2)
     
     
    Show all issues

    We're not doing anything with timezone in this function. I think we're missing a self.timezone = timezone?

    1. Looks like this isn't needed at all. Going to remove it.

  6. djblets/datagrid/grids.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    I think the type in the docs is supposed to be dict or or django.template.context.Context.

    1. It gets noramlized to a dict. The render context stuff is actually a bit wonky all around, because it can enter in as a dict or a Context, and we pass it around as being able to be either, but we actually store it as a dict. For now I'm going to keep this as-is, but I have plans to revisit some of this for some of the IDE integration work (where we're going to need to change some of these render flows).

  7. djblets/datagrid/grids.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Missing documentation for *args and **kwargs.

  8. 
      
chipx86
david
  1. Ship It!
  2. 
      
maubin
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.x (860c988)