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	ID	Author
fixing indent errors	3d387ef246879fc77562bd1e538f4562cb16f2d5	anahita-m
fixing formatting issue with imports	fa90587358957685d770fc673b75f939e8216a5c	anahita-m

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)

The issue has been resolved. 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	ID	Author
	replacing non-named usages to use names logging in files in reviewboard.notifications	44c1e3fcb19b8ff62372c0e68fc48011a6c49a1c	anahita-m
	replacing non-named usages to use names logging in files in reviewboard.notifications	bdab39b26ba2a4e812aade7404ad2cbe7c886e1d	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.

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
Please put that second blank line back.
```
reviewboard/notifications/email/utils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Each of these blank lines should be two.
```
reviewboard/notifications/tests/test_webhooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Please put this blank line back.
```
reviewboard/notifications/tests/test_webhooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We don't need a blank line here.
```
reviewboard/notifications/tests/test_webhooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This blank line can go away.
```
reviewboard/notifications/webhooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Please add another blank line here.
```

Commits:

	Summary	ID	Author
	replacing non-named usages to use names logging in files in reviewboard.notifications	bdab39b26ba2a4e812aade7404ad2cbe7c886e1d	anahita-m
	replacing non-named usages to use names logging in files in reviewboard.notifications	a0fabcec3e461847f60067342e03d607bd955993	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)
The issue has been resolved. Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W191 indentation contains tabs
```
reviewboard/notifications/email/message.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/notifications/email/message.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```

Commits:

	Summary	ID	Author
	replacing non-named usages to use names logging in files in reviewboard.notifications	a0fabcec3e461847f60067342e03d607bd955993	anahita-m
	replacing non-named usages to use names logging in files in reviewboard.notifications	20dd6e8ed1657cad1731740b7b2b2b43b3d9414f	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)
The issue has been resolved. Show all issues
```
E231 missing whitespace after ','
```
reviewboard/notifications/email/message.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/notifications/email/message.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 4)
The issue has been resolved. Show all issues
```
W191 indentation contains tabs
```

Commits:

	Summary	ID	Author
	replacing non-named usages to use names logging in files in reviewboard.notifications	20dd6e8ed1657cad1731740b7b2b2b43b3d9414f	anahita-m
	replacing non-named usages to use names logging in files in reviewboard.notifications	812da5d3aa499bf29c87297c8172a80add2c1617	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)
The issue has been resolved. Show all issues
```
W191 indentation contains tabs
```
reviewboard/notifications/email/message.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/notifications/email/message.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E101 indentation contains mixed spaces and tabs
```
reviewboard/notifications/email/message.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```

Commits:

	Summary	ID	Author
	replacing non-named usages to use names logging in files in reviewboard.notifications	812da5d3aa499bf29c87297c8172a80add2c1617	anahita-m
	replacing non-named usages to use names logging in files in reviewboard.notifications	35e775935ce42274cb5b4ccf567da9620e5abc49	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)

The issue has been resolved. 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	ID	Author
	replacing non-named usages to use names logging in files in reviewboard.notifications	35e775935ce42274cb5b4ccf567da9620e5abc49	anahita-m
	Replacing non-named usages to use names logging in files in reviewboard.notifications 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. 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	613df8f5030d136b519bae0de9e9fc812a2533f6	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)
The issue has been resolved. Show all issues
```
Please revert the changes to these lines.
```

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

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Dedent one space.
```
reviewboard/notifications/webhooks.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Dedent one space.
```

Commits:

Summary	ID	Author
Replacing non-named usages to use names logging in files in reviewboard.notifications 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. 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	613df8f5030d136b519bae0de9e9fc812a2533f6	anahita-m
fixing indent errors	3d387ef246879fc77562bd1e538f4562cb16f2d5	anahita-m
fixing formatting issue with imports	fa90587358957685d770fc673b75f939e8216a5c	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:: Completed
Change Summary:: Pushed to release-4.0.x (f1ff734)

~		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.

~		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.