Summary:

Added logger global variable to have named logging in files in reviewboard.attachments

Review Request #11394 — Created Jan. 22, 2021 and discarded Oct. 25, 2021, 12:43 p.m.

Information
Owner:	ryankang
Repository:	Review Board
Branch:	release-4.0.x
Bugs:	4900
Depends On:
Reviewers
Groups:	reviewboard, students
People:

Description

Previously logging in reviewboard/reviewboard/attachments was
un-named logging. This means it's hard to determine where the
error came from in the logs.

To fix this a global logger variable was defined in each file
that adds the location of the file to the error message.

Testing Done

Added unit test for unregister_mimetype_handler that passes it an
unregistered handler. This should cause an error to be logged. The
unit test listens to whether or not the logger ran.

Ran ./tests/runtests.py reviewboard.attachments and passed all 32 tests.

Commits

Summary
added logger global variable to have named logging in files in reviewboard/reviewboard/attachments/
added test that unregister handler logs when handler is unregistered
added two space after classes and fixed line that was too long
Fixed spacing of logger messages to align with start of open brackets
Changed test_unregister_handler_logger's docstring to use name of function being tested
added two blank spaces before and after logger definitions

Screenshots

Issues

Description	From	Last Updated
Please wrap your description and testing done to 80 columns. That last sentence in testing done also needs a period …	david	Jan. 23, 2021, 2:31 p.m.
Please add the bug number in the "bugs" field.	david	Jan. 23, 2021, 2:34 p.m.
Please capitalize the sentence in your summary. You can also refer to "reviewboard.attachments" instead of giving the file path.	david	Jan. 23, 2021, 2:52 p.m.
The diff as posted has a lot of extra stuff. It looks like you pulled on release-4.0.x but then didn't …	david	March 22, 2021, 3:23 p.m.
E302 expected 2 blank lines, found 1	reviewbot	Jan. 22, 2021, 1:32 p.m.
E302 expected 2 blank lines, found 1	reviewbot	Jan. 22, 2021, 1:32 p.m.
E501 line too long (111 > 79 characters)	reviewbot	Jan. 22, 2021, 1:33 p.m.
You'll need to update the indentation of the line with the arguments to match (since "logger" is one fewer character …	david	Jan. 23, 2021, 2:44 p.m.
Let's wrap this as such: """Testing if logger in unregister_mimetype_handler is called when an unregistered handler is passed in """ …	david	Jan. 23, 2021, 2:50 p.m.
Let's add another two blank lines above this.	david	March 2, 2021, 8:34 p.m.
This should have two blank lines above and below it.	david	March 2, 2021, 8:34 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/attachments/tests.py (Diff revision 1)
Show all issues
```
E302 expected 2 blank lines, found 1
```
reviewboard/attachments/tests.py (Diff revision 1)
Show all issues
```
E302 expected 2 blank lines, found 1
```
reviewboard/attachments/tests.py (Diff revision 1)
Show all issues
```
E501 line too long (111 > 79 characters)
```

Description:

~		Changed logging in `reviewboard/reviewboard/attachments/mimetypes.py` and `reviewboard/reviewboard/attachments/models.py` to named logging
	~	Changed logging in `reviewboard/reviewboard/attachments/mimetypes.py` and `reviewboard/reviewboard/attachments/models.py` to named logging
	+	Added new test in `MimetypeHandlerTests` that tests if an error log is called when `unregister_mimetype_handler` gets a handler that was never registered

Testing Done:

~		Added a test to `MimetypeHandlerTests` that ensures an error is logged if a handler that is already unregistered is passed to `unregister_mimetype_handler`
	~	Ran `./tests/runtests.py reviewboard.attachments` and passed all 32 tests

Commits:

	Summary
-	added logger global variable to have named logging in files in reviewboard/reviewboard/attachments/
-	added test that unregister handler logs when handler is unregistered
+	added logger global variable to have named logging in files in reviewboard/reviewboard/attachments/
+	added test that unregister handler logs when handler is unregistered
+	added two space after classes and fixed line that was too long

Diff:

Revision 2 (+67 -23)

Show changes

	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Groups:

+	students

Description:

~		Changed logging in `reviewboard/reviewboard/attachments/mimetypes.py` and `reviewboard/reviewboard/attachments/models.py` to named logging
~		Added new test in `MimetypeHandlerTests` that tests if an error log is called when `unregister_mimetype_handler` gets a handler that was never registered
	~	Previously logging in `reviewboard/reviewboard/attachments` was un-named logging. This means it's hard to determine where the error came from in the logs.
	~
	+	To fix this a global `logger` variable was defined in each file that adds the location of the file to the error message.

Testing Done:

	+	Added unit test for `unregister_mimetype_handler` that passes it an unregistered handler. This should cause an error to be logged. The unit test listens to whether or not the logger ran.
	+
		Ran `./tests/runtests.py reviewboard.attachments` and passed all 32 tests

Show all issues

Please wrap your description and testing done to 80 columns. That last sentence in testing done also needs a period at the end.

Show all issues

Please add the bug number in the "bugs" field.

Show all issues

Please capitalize the sentence in your summary. You can also refer to "reviewboard.attachments" instead of giving the file path.

reviewboard/attachments/mimetypes.py (Diff revision 2)

Show all issues

You'll need to update the indentation of the line with the arguments to match (since "logger" is one fewer character than "logging"). Here and all the other places you changed.

reviewboard/attachments/tests.py (Diff revision 2)

Show all issues

Let's wrap this as such:

"""Testing if logger in unregister_mimetype_handler is called when an
unregistered handler is passed in
"""

Note also that I changed the spaces to underscores in that message to match the method name.

Summary:

-	added logger global variable to have named logging in files in reviewboard/reviewboard/attachments/
+	Added logger global variable to have named logging in files in reviewboard.attachments

Description:

~		Previously logging in `reviewboard/reviewboard/attachments` was un-named logging. This means it's hard to determine where the error came from in the logs.
	~	Previously logging in `reviewboard/reviewboard/attachments` was
	+	un-named logging. This means it's hard to determine where the
	+	error came from in the logs.

~		To fix this a global `logger` variable was defined in each file that adds the location of the file to the error message.
	~	To fix this a global `logger` variable was defined in each file
	+	that adds the location of the file to the error message.

Testing Done:

~		Added unit test for `unregister_mimetype_handler` that passes it an unregistered handler. This should cause an error to be logged. The unit test listens to whether or not the logger ran.
	~	Added unit test for `unregister_mimetype_handler` that passes it an
	+	unregistered handler. This should cause an error to be logged. The
	+	unit test listens to whether or not the logger ran.

~		Ran `./tests/runtests.py reviewboard.attachments` and passed all 32 tests
	~	Ran `./tests/runtests.py reviewboard.attachments` and passed all 32 tests.

Commits:

	Summary
-	added logger global variable to have named logging in files in reviewboard/reviewboard/attachments/
-	added test that unregister handler logs when handler is unregistered
-	added two space after classes and fixed line that was too long
+	added logger global variable to have named logging in files in reviewboard/reviewboard/attachments/
+	added test that unregister handler logs when handler is unregistered
+	added two space after classes and fixed line that was too long
+	Fixed spacing of logger messages to align with start of open brackets
+	Changed test_unregister_handler_logger's docstring to use name of function being tested

Bugs:

+	4900

Diff:

Revision 3 (+94 -52)

Show changes

	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/attachments/mimetypes.py (Diff revision 3)
Show all issues
```
Let's add another two blank lines above this.
```
reviewboard/attachments/models.py (Diff revision 3)
Show all issues
```
This should have two blank lines above and below it.
```

Change Summary:

added 2 blank spaces before and after all definitions of logger

Commits:

	Summary
-	added logger global variable to have named logging in files in reviewboard/reviewboard/attachments/
-	added test that unregister handler logs when handler is unregistered
-	added two space after classes and fixed line that was too long
-	Fixed spacing of logger messages to align with start of open brackets
-	Changed test_unregister_handler_logger's docstring to use name of function being tested
+	added logger global variable to have named logging in files in reviewboard/reviewboard/attachments/
+	added test that unregister handler logs when handler is unregistered
+	added two space after classes and fixed line that was too long
+	Fixed spacing of logger messages to align with start of open brackets
+	Changed test_unregister_handler_logger's docstring to use name of function being tested
+	added two blank spaces before and after logger definitions

Diff:

Revision 4 (+1702 -4519)

Show changes

	AUTHORS
	contrib/docker/Dockerfile
	contrib/docker/build.py
	contrib/docker/examples/docker-compose.mysql.yaml
	contrib/docker/examples/docker-compose.postgres.yaml
	contrib/docker/examples/nginx_templates/reviewboard.conf.template
	contrib/docker/examples/postgres/init-reviewboard-db.sh
	contrib/docker/packages/.gitignore
	76 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

Show all issues

The diff as posted has a lot of extra stuff. It looks like you pulled on release-4.0.x but then didn't rebase your change. Please rebase and re-post. It's always a good idea to double check the diff view before you publish the update to make sure it looks like what you want.

Review Board 6.0 beta 2 (dev)

Added logger global variable to have named logging in files in reviewboard.attachments

flake8

Description:

Testing Done:

Commits:

Diff:

Checks run (2 succeeded)

Groups:

Description:

Testing Done:

Summary:

Description:

Testing Done:

Commits:

Bugs:

Diff:

Checks run (2 succeeded)

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Status: Discarded