Summary

Add typing and fix issues throughout the datagrids code.

Review Request #13355 — Created Oct. 17, 2023 and submitted Oct. 27, 2023, 4:18 p.m.

Information

Owner

chipx86

Repository

Djblets

Branch

release-3.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

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.

Testing Done

Djblets and Review Board unit tests pass.

Tested all of the datagrids used in Review Board.

Commits

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

Summary

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

Issues

Description	From	Last Updated
Hold off on this. Some corner cases to resolve.	chipx86	Oct. 17, 2023, 10:29 p.m.
Missing the type.	maubin	Oct. 26, 2023, 5:30 p.m.
Missing the type.	maubin	Oct. 26, 2023, 5:30 p.m.
While you're here could you fix this typo: "conver" -> "convert".	maubin	Oct. 26, 2023, 5:30 p.m.
We're not doing anything with timezone in this function. I think we're missing a self.timezone = timezone?	maubin	Oct. 26, 2023, 5:36 p.m.
I think the type in the docs is supposed to be dict or or django.template.context.Context.	maubin	Oct. 26, 2023, 5:33 p.m.
Missing documentation for args and *kwargs.	maubin	Oct. 26, 2023, 5:34 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Hold off on this. Some corner cases to resolve.

Change Summary:

Fixed some corner cases due to missing state and some checks for falsiness that should have checked against None.

Commits:

	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.	ad85e5e402b0e0aef4af9a34080431ee103f3ba4
	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.	18e9af9ae93bbbb77f5b8b72df8a1dffcb8891dd

Diff:

Revision 2 (+2274 -662)

Show changes

djblets/datagrid/grids.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/datagrid/grids.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing the type.
```
djblets/datagrid/grids.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing the type.
```
djblets/datagrid/grids.py (Diff revision 2)
The issue has been resolved. Show all issues
```
While you're here could you fix this typo: "conver" -> "convert".
```

djblets/datagrid/grids.py (Diff revision 2)

The issue has been resolved. Show all issues

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

chipx86 Oct. 26, 2023, 5:36 p.m.

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

djblets/datagrid/grids.py (Diff revision 2)

The issue has been dropped. Show all issues

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

chipx86 Oct. 26, 2023, 5:36 p.m.

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).

djblets/datagrid/grids.py (Diff revision 2)

The issue has been resolved. Show all issues

Missing documentation for *args and **kwargs.

Change Summary:


Fixed some missing types and argument docs, and fixed typos.
Removed the unused timezone argument from DateTimeSinceColumn. This was never used, and likely was copy-pasted from DateTimeColumn.

Commits:

	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.	18e9af9ae93bbbb77f5b8b72df8a1dffcb8891dd
	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

Diff:

Revision 3 (+2294 -666)

Show changes

djblets/datagrid/grids.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.x (860c988)