Summary

Add typing for ConcurrencyManager and deprecate it.

Review Request #14044 — Created July 16, 2024 and submitted Aug. 13, 2024, 8:41 a.m.

Information

Owner

david

Repository

Djblets

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

This change adds generic parameters for ConcurrencyManager so it can be

used where we would expect a Manager.
This particular class also hasn't been useful since the very early days

of Django, since the base Manager.get_or_create implementation now does

exactly what this code does, preventing us from ever hitting the

exception handler. Because of that, I've removed the get_or_create

implementation entirely and marked the class as deprecated.

Testing Done


Ran unit tests.

Commits

Summary	ID
Add typing for ConcurrencyManager and deprecate it. This change adds generic parameters for `ConcurrencyManager` so it can be used where we would expect a Manager. This particular class also hasn't been useful since the very early days of Django, since the base `Manager.get_or_create` implementation now does exactly what this code does, preventing us from ever hitting the exception handler. Because of that, I've removed the `get_or_create` implementation entirely and marked the class as deprecated. Testing Done: Used with some other code.	6975e7645864bcb3cd0919f2069a68441e38c3df

Summary

Add typing for ConcurrencyManager and deprecate it.

This change adds generic parameters for `ConcurrencyManager` so it can be used where we would expect a Manager. This particular class also hasn't been useful since the very early days of Django, since the base `Manager.get_or_create` implementation now does exactly what this code does, preventing us from ever hitting the exception handler. Because of that, I've removed the `get_or_create` implementation entirely and marked the class as deprecated. Testing Done: Used with some other code.

6975e7645864bcb3cd0919f2069a68441e38c3df

Issues

Description	From	Last Updated
"Django"	chipx86	July 30, 2024, 9:44 a.m.
We should reference the function properly for the Django manager method., while we're fixing the docs. Can we also add …	chipx86	July 30, 2024, 9:44 a.m.
With Manager[_T_co], do we need Generic[...]? I think we inherit the generic nature from the former, given the TypeVar.	chipx86	July 31, 2024, 8:20 a.m.
I think we've been moving more to a two-release deprecation cycle for most things.	chipx86	July 31, 2024, 8:20 a.m.
Can we state that it wil be removed in 8.0?	chipx86	Aug. 12, 2024, 9:11 a.m.

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

djblets/db/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Django"
```

djblets/db/managers.py (Diff revision 1)

The issue has been resolved. Show all issues

We should reference the function properly for the Django manager method., while we're fixing the docs.
Can we also add Args and Returns?

djblets/db/managers.py (Diff revision 1)

Huh, wait, why has this been working all this time? We should have noticed this.

david July 29, 2024, 4:50 p.m.

I just looked at the source for Django's get_or_create and they're doing this same thing internally now. I think we can just deprecate this entirely.

chipx86 July 29, 2024, 5:10 p.m.

Decided to dive in and I concur. Looks like the old logic we were working around existed in 0.95, but by 1.0 they implemented the first version of the equivalant of what this class does (wow, this is old). We probably can deprecate this.

Summary:

Add typing for ConcurrencyManager.

Add typing for ConcurrencyManager and deprecate it.

Description:

		This change adds generic parameters for `ConcurrencyManager` so it can be
~		used where we would expect a Manager. It also fixes the fallback return
~		for `get_or_create()` so we're always returning what a caller would expect
~		from this method.
	~	used where we would expect a Manager.
	~
	~	This particular class also hasn't been useful since the very early days
	+	of Django, since the base `Manager.get_or_create` implementation now does
	+	exactly what this code does, preventing us from ever hitting the
	+	exception handler. Because of that, I've removed the `get_or_create`
	+	implementation entirely and marked the class as deprecated.

Testing Done:

~		Used with some other code.
	~	Ran unit tests.

Commits:

	Summary	ID
	Add typing for ConcurrencyManager. This change adds generic parameters for `ConcurrencyManager` so it can be used where we would expect a Manager. It also fixes the fallback return for `get_or_create()` so we're always returning what a caller would expect from this method. Testing Done: Used with some other code.	c44583233a5b550b78e9305ad110ca3927e0cb65
	Add typing for ConcurrencyManager and deprecate it. This change adds generic parameters for `ConcurrencyManager` so it can be used where we would expect a Manager. This particular class also hasn't been useful since the very early days of Django, since the base `Manager.get_or_create` implementation now does exactly what this code does, preventing us from ever hitting the exception handler. Because of that, I've removed the `get_or_create` implementation entirely and marked the class as deprecated. Testing Done: Used with some other code.	eb69746a038570ca9f0ad9c1f0e95a87f9e9240f

Diff:

Revision 2 (+54 -44)

Show changes

djblets/db/managers.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/db/managers.py (Diff revision 2)

The issue has been resolved. Show all issues

With Manager[_T_co], do we need Generic[...]? I think we inherit the generic nature from the former, given the TypeVar.

djblets/db/managers.py (Diff revision 2)

The issue has been resolved. Show all issues

I think we've been moving more to a two-release deprecation cycle for most things.

Commits:

	Summary	ID
	Add typing for ConcurrencyManager and deprecate it. This change adds generic parameters for `ConcurrencyManager` so it can be used where we would expect a Manager. This particular class also hasn't been useful since the very early days of Django, since the base `Manager.get_or_create` implementation now does exactly what this code does, preventing us from ever hitting the exception handler. Because of that, I've removed the `get_or_create` implementation entirely and marked the class as deprecated. Testing Done: Used with some other code.	eb69746a038570ca9f0ad9c1f0e95a87f9e9240f
	Add typing for ConcurrencyManager and deprecate it. This change adds generic parameters for `ConcurrencyManager` so it can be used where we would expect a Manager. This particular class also hasn't been useful since the very early days of Django, since the base `Manager.get_or_create` implementation now does exactly what this code does, preventing us from ever hitting the exception handler. Because of that, I've removed the `get_or_create` implementation entirely and marked the class as deprecated. Testing Done: Used with some other code.	6975e7645864bcb3cd0919f2069a68441e38c3df

Diff:

Revision 3 (+74 -44)

Show changes

	djblets/deprecation.py
	djblets/db/managers.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

djblets/db/managers.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we state that it wil be removed in 8.0?
```

Status:: Completed
Change Summary:: Pushed to release-6.x (7e50db2)