Summary

Use .set() when assigning multiple relations.

Review Request #11989 — Created Jan. 24, 2022 and submitted Feb. 7, 2022, 11:07 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Using the assignment operator for setting the far side of a relation
such as ManyToManyField or ForeignKey now generates an error
instructing us to use .set() instead.

In many cases, we were using .set() to set a list of relations on
newly-created objects within tests. In these cases, we can save one
query per use by using .add() instead.

Testing Done

Ran unit tests.

Commits

Summary	ID
Use .set() when assigning multiple relations. Using the assignment operator for setting the far side of a relation such as `ManyToManyField` or `ForeignKey` now generates an error instructing us to use `.set()` instead. In many cases, we were using `.set()` to set a list of relations on newly-created objects within tests. In these cases, we can save one query per use by using `.add()` instead. Testing Done: Ran unit tests.	4865f784cdf233789f9e9ccc96528bf589b09ac6

Summary

Use .set() when assigning multiple relations.

Using the assignment operator for setting the far side of a relation such as `ManyToManyField` or `ForeignKey` now generates an error instructing us to use `.set()` instead. In many cases, we were using `.set()` to set a list of relations on newly-created objects within tests. In these cases, we can save one query per use by using `.add()` instead. Testing Done: Ran unit tests.

4865f784cdf233789f9e9ccc96528bf589b09ac6

Issues

Description	From	Last Updated
Purely optional work (because I'm sure you're really hoping for more things to do), but something that's stood out for …	chipx86	Jan. 26, 2022, 2:41 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Jan. 24, 2022, 3:38 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Jan. 24, 2022, 3:38 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```

Commits:

	Summary	ID
	Use .set() when assigning multiple relations. Using the assignment operator for setting the far side of a relation such as `ManyToManyField` or `ForeignKey` now generates an error instructing us to use `.set()` instead. This is a relatively straightforward update. Testing Done: Ran unit tests.	a06e10626df204c34ea172234458e8355dea536e
	Use .set() when assigning multiple relations. Using the assignment operator for setting the far side of a relation such as `ManyToManyField` or `ForeignKey` now generates an error instructing us to use `.set()` instead. This is a relatively straightforward update. Testing Done: Ran unit tests.	d830b7b3ea93f8daa4a7b54dea5973ab4ebaeafa

Diff:

Revision 2 (+334 -280)

Show changes

	reviewboard/accounts/tests/test_user.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/notifications/tests/test_email_utils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_conditions.py
	reviewboard/reviews/tests/test_counters.py
	12 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

The issue has been resolved. Show all issues

Purely optional work (because I'm sure you're really hoping for more things to do), but something that's stood out for a long time (and this would feel like the right time to fix it) is that our unit tests are using = (or now .set()) when the probably shouldn't.
=/set() will first delete all entries on the relation and then replace them. This is one more SQL query than we usually need in tests. We could (ever so slightly) speed up the test suite by using .add() instead in most cases.
But I'm also fine not worrying about it at all for this change. Mostly putting that out there and on the radar.

Description:

		Using the assignment operator for setting the far side of a relation
		such as `ManyToManyField` or `ForeignKey` now generates an error
~		instructing us to use `.set()` instead. This is a relatively
~		straightforward update.
	~	instructing us to use `.set()` instead.
	~
	+	In many cases, we were using `.set()` to set a list of relations on
	+	newly-created objects within tests. In these cases, we can save one
	+	query per use by using `.add()` instead.

Commits:

	Summary	ID
	Use .set() when assigning multiple relations. Using the assignment operator for setting the far side of a relation such as `ManyToManyField` or `ForeignKey` now generates an error instructing us to use `.set()` instead. This is a relatively straightforward update. Testing Done: Ran unit tests.	d830b7b3ea93f8daa4a7b54dea5973ab4ebaeafa
	Use .set() when assigning multiple relations. Using the assignment operator for setting the far side of a relation such as `ManyToManyField` or `ForeignKey` now generates an error instructing us to use `.set()` instead. In many cases, we were using `.set()` to set a list of relations on newly-created objects within tests. In these cases, we can save one query per use by using `.add()` instead. Testing Done: Ran unit tests.	4865f784cdf233789f9e9ccc96528bf589b09ac6

Diff:

Revision 3 (+326 -310)

Show changes

	reviewboard/accounts/tests/test_user.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/notifications/tests/test_email_utils.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_conditions.py
	reviewboard/reviews/tests/test_counters.py
	13 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status: Closed (submitted)

Change Summary:

Pushed to django-3.2 (8e434f5)