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

Review Request #11396 — Created Jan. 22, 2021 and updated

amohapatra
Review Board
release-4.0.x
4907
reviewboard, students

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.

Summary Author
Replacing non-named usages to use names logging in files in reviewboard.notifications
anahita-m
Description From Last Updated

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

daviddavid

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

ryankangryankang

Please put that second blank line back.

daviddavid

Each of these blank lines should be two.

daviddavid

Please put this blank line back.

daviddavid

We don't need a blank line here.

daviddavid

This blank line can go away.

daviddavid

Please add another blank line here.

daviddavid

E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

W191 indentation contains tabs

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

W191 indentation contains tabs

reviewbotreviewbot

W191 indentation contains tabs

reviewbotreviewbot

E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

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

daviddavid

Please revert the changes to these lines.

daviddavid

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

daviddavid

Dedent one space.

daviddavid

Dedent one space.

daviddavid

Dedent one space.

daviddavid

Dedent one space.

daviddavid

Dedent one space.

daviddavid

Dedent one space.

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

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

  3. 
      
amohapatra
david
  1. 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.

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

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

    2. 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?

  3. reviewboard/notifications/email/message.py (Diff revision 2)
     
     
     
     
     

    Please put that second blank line back.

  4. reviewboard/notifications/email/utils.py (Diff revision 2)
     
     
     
     
     
     

    Each of these blank lines should be two.

  5. Please put this blank line back.

  6. We don't need a blank line here.

  7. This blank line can go away.

  8. reviewboard/notifications/webhooks.py (Diff revision 2)
     
     
     
     

    Please add another blank line here.

  9. 
      
amohapatra
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

amohapatra
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

amohapatra
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

amohapatra
amohapatra
amohapatra
david
  1. 
      
  2. reviewboard/notifications/webhooks.py (Diff revision 6)
     
     
     
     
     

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

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

  3. 
      
amohapatra
amohapatra
Review request changed

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.

david
  1. Just some trivial formatting-related things.

  2. reviewboard/notifications/email/message.py (Diff revision 7)
     
     
     
     
     
     

    Please revert the changes to these lines.

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

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

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

    Dedent one space.

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

    Dedent one space.

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

    Dedent one space.

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

    Dedent one space.

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

    Dedent one space.

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

    Dedent one space.

  10. 
      
Loading...