Summary:

Replacing non-named usages to use names logging in files in reviewboard.notifications

Review Request #11396 — Created Jan. 22, 2021 and submitted Aug. 25, 2021, 3:50 p.m.

Information
Owner:	amohapatra
Repository:	Review Board
Branch:	master
Bugs:	4907
Depends On:
Reviewers
Groups:	reviewboard, students
People:

Description

This commit is replacing non-named usages in reviewboard/notifications to use
named logging instead. The purpose of this change is to automatically include
more information about the module where the logging message came from so that
the messages can be of more use when diagnosing problems from logs.

Testing Done

I ran the unit tests under reviewboard/notifications/tests by running
./tests/runtests.py reviewboard.notifications.tests and confirmed that all
162 tests ran without fail.

Commits

Summary	Author
fixing indent errors	anahita-m
fixing formatting issue with imports	anahita-m

Screenshots

Issues

Description	From	Last Updated
Can you wrap your description and testing done to be 80 characters in width?	david	Feb. 6, 2021, 7:39 a.m.
If you are only importing logger from reviewboard.notifications.webhooks shouldn't you import it once at the top of the file?	ryankang	Jan. 22, 2021, 2:48 p.m.
Please put that second blank line back.	david	Jan. 23, 2021, 10:35 a.m.
Each of these blank lines should be two.	david	Jan. 23, 2021, 10:35 a.m.
Please put this blank line back.	david	Jan. 23, 2021, 10:35 a.m.
We don't need a blank line here.	david	Jan. 23, 2021, 10:35 a.m.
This blank line can go away.	david	Jan. 23, 2021, 10:35 a.m.
Please add another blank line here.	david	Jan. 23, 2021, 10:35 a.m.
E101 indentation contains mixed spaces and tabs	reviewbot	Jan. 23, 2021, 10:36 a.m.
W191 indentation contains tabs	reviewbot	Jan. 23, 2021, 10:36 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Jan. 23, 2021, 10:36 a.m.
E101 indentation contains mixed spaces and tabs	reviewbot	Jan. 23, 2021, 10:36 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Jan. 23, 2021, 10:37 a.m.
E231 missing whitespace after ','	reviewbot	Jan. 23, 2021, 11:09 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Jan. 23, 2021, 11:09 a.m.
E101 indentation contains mixed spaces and tabs	reviewbot	Jan. 23, 2021, 11:09 a.m.
W191 indentation contains tabs	reviewbot	Jan. 23, 2021, 11:09 a.m.
W191 indentation contains tabs	reviewbot	Jan. 23, 2021, 10:36 a.m.
E101 indentation contains mixed spaces and tabs	reviewbot	Jan. 23, 2021, 10:36 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Jan. 23, 2021, 10:36 a.m.
E101 indentation contains mixed spaces and tabs	reviewbot	Jan. 23, 2021, 10:36 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Jan. 23, 2021, 10:36 a.m.
Indentation will need to be fixed here, since logger is one character shorter than logging. Please check all your changed …	david	Feb. 6, 2021, 7:40 a.m.
Please revert the changes to these lines.	david	April 8, 2021, 7 p.m.
These lines need to be dedented one space, because "logger" is one character shorter than "logging".	david	April 8, 2021, 7 p.m.
Dedent one space.	david	April 8, 2021, 7 p.m.
Dedent one space.	david	April 8, 2021, 7 p.m.
Dedent one space.	david	April 8, 2021, 7 p.m.
Dedent one space.	david	April 8, 2021, 7 p.m.
Dedent one space.	david	April 8, 2021, 7 p.m.
Dedent one space.	david	April 8, 2021, 6:36 p.m.

flake8 passed.

JSHint passed.

Groups:

+	students

Testing Done:

~		I ran the unit tests under reviewboard/notifications/tests and confirmed that all 162 tests ran without fail.
	~	I ran the unit tests under reviewboard/notifications/tests by running `./tests/runtests.py reviewboard.notifications.tests` and confirmed that all 162 tests ran without fail.

Summary:

-	replacing non-named usages to use names logging in files in reviewboard.notifications
+	Replacing non-named usages to use names logging in files in reviewboard.notifications

reviewboard/notifications/tests/test_webhooks.py (Diff revision 1)

Show all issues

If you are only importing logger from reviewboard.notifications.webhooks shouldn't you import it once at the top of the file?

amohapatra Jan. 22, 2021, 2:48 p.m.

Yeah, I think you're right. I didn't notice that all of the methods being tested were coming from the same file and same logger. I just updated with new changes where I removed the localized import logger statement from those methods and moved the import to the top of the file instead.

Commits:

	Summary	Author
-	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m
+	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m

Diff:

Revision 2 (+76 -64)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/notifications/tests/test_webhooks.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

This looks pretty good so far. I have a bunch of comments but they all relate to the placement of blank lines. The most confusing part of it is that Python specifies that at the top-level scope there should be two blank lines between definitions.

Show all issues

Can you wrap your description and testing done to be 80 characters in width?

david Jan. 24, 2021, 9:33 a.m.

These still need to be rewrapped (maybe you fixed it in your local commit?)

amohapatra Jan. 24, 2021, 10:55 p.m.

I think I was able to fix it finally, could you let me know if I did it correctly this time before I close the issue?

reviewboard/notifications/email/message.py (Diff revision 2)
Show all issues
```
Please put that second blank line back.
```
reviewboard/notifications/email/utils.py (Diff revision 2)
Show all issues
```
Each of these blank lines should be two.
```
reviewboard/notifications/tests/test_webhooks.py (Diff revision 2)
Show all issues
```
Please put this blank line back.
```
reviewboard/notifications/tests/test_webhooks.py (Diff revision 2)
Show all issues
```
We don't need a blank line here.
```
reviewboard/notifications/tests/test_webhooks.py (Diff revision 2)
Show all issues
```
This blank line can go away.
```
reviewboard/notifications/webhooks.py (Diff revision 2)
Show all issues
```
Please add another blank line here.
```

Commits:

	Summary	Author
-	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m
+	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m

Diff:

Revision 3 (+98 -72)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/notifications/tests/test_webhooks.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/email/message.py (Diff revision 3)
Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 3)
Show all issues
```
W191 indentation contains tabs
```
reviewboard/notifications/email/message.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/notifications/email/message.py (Diff revision 3)
Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```

Commits:

	Summary	Author
-	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m
+	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m

Diff:

Revision 4 (+96 -72)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/notifications/tests/test_webhooks.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/email/message.py (Diff revision 4)
Show all issues
```
E231 missing whitespace after ','
```
reviewboard/notifications/email/message.py (Diff revision 4)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/notifications/email/message.py (Diff revision 4)
Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 4)
Show all issues
```
W191 indentation contains tabs
```

Commits:

	Summary	Author
-	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m
+	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m

Diff:

Revision 5 (+98 -72)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/notifications/tests/test_webhooks.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/email/message.py (Diff revision 5)
Show all issues
```
W191 indentation contains tabs
```
reviewboard/notifications/email/message.py (Diff revision 5)
Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 5)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/notifications/email/message.py (Diff revision 5)
Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 5)
Show all issues
```
E128 continuation line under-indented for visual indent
```

Commits:

	Summary	Author
-	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m
+	replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m

Diff:

Revision 6 (+96 -72)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/notifications/tests/test_webhooks.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Testing Done:

~		I ran the unit tests under reviewboard/notifications/tests by running `./tests/runtests.py reviewboard.notifications.tests` and confirmed that all 162 tests ran without fail.
	~	I ran the unit tests under reviewboard/notifications/tests by running
	+	`./tests/runtests.py reviewboard.notifications.tests` and confirmed that all 162 tests ran without fail.

Bugs:

+	4907

reviewboard/notifications/webhooks.py (Diff revision 6)

Show all issues

Indentation will need to be fixed here, since logger is one character shorter than logging. Please check all your changed instances for this.

amohapatra Jan. 24, 2021, 10:55 p.m.

I'm probably not understanding this correctly as I wasn't able to find any errors with indentation when I went through all the changed instances. I also ran flake8 on the changed files and did not see any errors regarding indentation.

Change Summary:

fixing description and testing done text wrap around 80 chars

Commits:

	Summary		Author
-		replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m
+		Replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m

Diff:

Revision 7 (+96 -72)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/notifications/tests/test_webhooks.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

~		This commit is replacing non-named usages in reviewboard/notifications to use named logging instead. The purpose of this change is to automatically include more information about the module where the logging message came from so that the messages can be of more use when diagnosing problems from logs.
	~	This commit is replacing non-named usages in reviewboard/notifications to use
	+	named logging instead. The purpose of this change is to automatically include
	+	more information about the module where the logging message came from so that
	+	the messages can be of more use when diagnosing problems from logs.

Testing Done:

		I ran the unit tests under reviewboard/notifications/tests by running
~		`./tests/runtests.py reviewboard.notifications.tests` and confirmed that all 162 tests ran without fail.
	~	`./tests/runtests.py reviewboard.notifications.tests` and confirmed that all
	+	162 tests ran without fail.

Just some trivial formatting-related things.

reviewboard/notifications/email/message.py (Diff revision 7)
Show all issues
```
Please revert the changes to these lines.
```

reviewboard/notifications/webhooks.py (Diff revision 7)

Show all issues

These lines need to be dedented one space, because "logger" is one character shorter than "logging".

reviewboard/notifications/webhooks.py (Diff revision 7)
Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
Show all issues
```
Dedent one space.
```

Commits:

		Author
-	Replacing non-named usages to use names logging in files in reviewboard.notifications	anahita-m
+	fixing indent errors	anahita-m
+	fixing formatting issue with imports	anahita-m

Branch:

-	release-4.0.x
+	master

Diff:

Revision 8 (+32 -34)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/tests/test_email_sending.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

Going to make some fixes and get this landed. Thanks!

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (f1ff734)