Summary

Use named modules for integration modules logger.

Review Request #11812 — Created Sept. 17, 2021 and submitted March 24, 2022, 2: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	ID	Author
Use named modules for integration modules logger. 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 yourself depending 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	7ee09538467192de2355dca8f7af3795263a3860	Jordan

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, 2: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, 3: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, 3:21 p.m.
Similarly, two blank lines between the logger = and the class definition.	chipx86	Sept. 17, 2021, 3: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, 3:22 p.m.
Same with the other file, there should be two blank lines on either side of this declaration.	chipx86	Sept. 17, 2021, 3: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, 3:22 p.m.
This could also be changed to assertSpyCalled(), like above.	chipx86	Sept. 17, 2021, 3: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, 2:27 p.m.

flake8 passed.

JSHint passed.

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

The issue has been resolved. 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.

The issue has been resolved. 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)

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
Similarly, two blank lines between the logger = and the class definition.
```

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

The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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)
The issue has been resolved. 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	ID	Author
	Use named modules for integration modules logger. An easyfix bug (ticket #4906) suggested that a logger can be more useful if it is named using the built in __name__variable. models.py is the only file (excluding tests) that uses the logger, so now the logger within that part of the code will be called "reviewboard.integrations.models". This change was tested by running all of the python unit tests. This change did require an alteration to the unit test to ensure that it knew which logger to be watching for changes on. Testing Done: - Ran all of the python unit tests once the changes were added	ee45a589b67befdeca3deb7b6c5896fef182c1fb	Jordan
	Use named modules for integration modules logger. 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 yourself depending 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	b1456e9adaa1fc5871837854ba5ca0532ec2c868	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)

The issue has been resolved. 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	ID	Author
	Use named modules for integration modules logger. 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 yourself depending 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	b1456e9adaa1fc5871837854ba5ca0532ec2c868	Jordan
	Use named modules for integration modules logger. 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 yourself depending 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	7ee09538467192de2355dca8f7af3795263a3860	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)