Summary:

Use named modules for integration modules logger.

Review Request #11812 — Created Sept. 17, 2021 and submitted March 24, 2022, 1:03 p.m.

Information
Owner:	jordanvandenbruel
Repository:	Review Board
Branch:	release-4.0.x
Bugs:	4906
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

When using a logger, one of the challenges that you can run into is
determining where a logger is being called. This uncertainty can be
removed by providing the logger with a name. While you could choose
to name loggers yourselfdepending on the usecase, the recommended
convention is to use Python's __name__ variable to easily identify
where the logger is acting. One instance within the codebase that
used an unnamed logger was within reviewboard/reviewboard/integrations/.

models.py is the only file (excluding tests) within /integrations/ that
uses a logger, so it has been renamed with
__name__ to reviewboard.integrations.models.

This update did require a change to test_configs within integrations as well
to ensure it knew which logger to listen to.

Testing Done

- Ran all of the python unit tests once the changes were added

Commits

Summary		Author
	Use named modules for integration modules logger.	Jordan

Screenshots

Issues

Description	From	Last Updated
"Testing Done" has the text "Testing Done:" within it, which will end up landing as: Testing Done: Testing Done: - …	chipx86	Sept. 17, 2021, 1:32 p.m.
A few nits in the description: For things like __name__ or file paths, use backticks around the text. That'll make …	chipx86	Sept. 17, 2021, 2:21 p.m.
You'll want to maintain two blank lines between top-level sections of the file (import groups and declarations being their own …	chipx86	Sept. 17, 2021, 2:21 p.m.
Similarly, two blank lines between the logger = and the class definition.	chipx86	Sept. 17, 2021, 2:21 p.m.
The other lines in this statement no longer align with the start of the parameter on the first line.	chipx86	Sept. 17, 2021, 2:22 p.m.
Same with the other file, there should be two blank lines on either side of this declaration.	chipx86	Sept. 17, 2021, 2:22 p.m.
Since this code is changing anyway, it'd be nice to move it to the modern KGB spy checks. We haven't …	chipx86	Sept. 17, 2021, 2:22 p.m.
This could also be changed to assertSpyCalled(), like above.	chipx86	Sept. 17, 2021, 2:22 p.m.
Looks like the parameters to this statement still aren't aligned. Once that's fixed, we'll be good to land! :)	chipx86	Sept. 18, 2021, 1:27 p.m.

flake8 passed.

JSHint passed.

```
Congrats on your first change! :)
```

Show all issues

"Testing Done" has the text "Testing Done:" within it, which will end up landing as:

Testing Done:
Testing Done:
- Ran all ...

You can remove the "Testing Done:" text.

Show all issues

A few nits in the description:

For things like __name__ or file paths, use backticks around the text. That'll make it stand out as code/literals.
Descriptions should wrap to somewhere around 75 characters, to best fit in Git commit messages.
Rather than providing the ticket number in the description, fill it out in the Bugs field on the right.

A general tip about writing these. While it's true that the ticket suggests this, it's probably more helpful if you let that be separate, and instead keep the description of the change self-contained. A good approach is to describe it in problem/solution form, where you describe the problem that needed solving, and then how it's been solved. You can find example of that in our guide or other commits.

reviewboard/integrations/models.py (Diff revision 1)

Show all issues

You'll want to maintain two blank lines between top-level sections of the file (import groups and declarations being their own sections), just like how there previously were two between import groups and the class definition.

reviewboard/integrations/models.py (Diff revision 1)

Show all issues

Similarly, two blank lines between the logger = and the class definition.

reviewboard/integrations/models.py (Diff revision 1)

Show all issues

The other lines in this statement no longer align with the start of the parameter on the first line.

reviewboard/integrations/tests/test_configs.py (Diff revision 1)

Show all issues

Same with the other file, there should be two blank lines on either side of this declaration.

reviewboard/integrations/tests/test_configs.py (Diff revision 1)

Show all issues

Since this code is changing anyway, it'd be nice to move it to the modern KGB spy checks.
We haven't talked about KGB yet, but we'll bring it up during the sprint. It's a unit test helping library we maintain that helps check when functions have been called, check how they've been called, and override functionality. We have a guide and function reference at https://github.com/beanbaginc/kgb
For this, you should be able to change these assertTrue statements to:

self.assertSpyCalled(logger.debug)


etc.

reviewboard/integrations/tests/test_configs.py (Diff revision 1)
Show all issues
```
This could also be changed to assertSpyCalled(), like above.
```

Description:

~		An easyfix bug (ticket #4906) suggested that a logger can be more useful if it is named using the built in __name__ variable. The bug fix specifically mentioned that this should be implemented within 'reviewboard/reviewboard/integrations/'.
	~	When using a logger, one of the challenges that you can run into is
	+	determining where a logger is being called. This uncertainty can be
	+	removed by providing the logger with a name. While you could choose
	+	to name loggers yourselfdepending on the usecase, the recommended
	+	convention is to use Python's `__name__` variable to easily identify
	+	where the logger is acting. One instance within the codebase that
	+	used an unnamed logger was within `reviewboard/reviewboard/integrations/`.

~		models.py is the only file (excluding tests) that uses the logger within integrations, so now the logger within that part of the code will be called "reviewboard.integrations.models".
	~	models.py is the only file (excluding tests) within `/integrations/` that
	+	uses a logger, so it has been renamed with
	+	`__name__` to `reviewboard.integrations.models`.

~		This update did require a change to the configs unit tests within integrations as well to ensure it knew which logger to listen to.
	~	This update did require a change to `test_configs` within integrations as well
	+	to ensure it knew which logger to listen to.

Testing Done:

~		Testing Done:
	~	- Ran all of the python unit tests once the changes were added
-		- Ran all of the python unit tests once the changes were added

Commits:

	Summary		Author
-		Use named modules for integration modules logger.	Jordan
+		Use named modules for integration modules logger.	Jordan

Bugs:

+	4906

Diff:

Revision 2 (+30 -18)

Show changes

	reviewboard/integrations/models.py
	reviewboard/integrations/tests/test_configs.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/integrations/models.py (Diff revision 2)

Show all issues

Looks like the parameters to this statement still aren't aligned. Once that's fixed, we'll be good to land! :)

Commits:

	Summary		Author
-		Use named modules for integration modules logger.	Jordan
+		Use named modules for integration modules logger.	Jordan

Diff:

Revision 3 (+34 -22)

Show changes

	reviewboard/integrations/models.py
	reviewboard/integrations/tests/test_configs.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status: Closed (submitted)

Change Summary:

Pushed to django-3.2 (0efa050)