Use named modules for integration modules logger.

Review Request #11812 — Created Sept. 17, 2021 and submitted

Information

Review Board
release-4.0.x

Reviewers

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.

- Ran all of the python unit tests once the changes were added

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
Description From Last Updated

"Testing Done" has the text "Testing Done:" within it, which will end up landing as: Testing Done: Testing Done: - …

chipx86chipx86

A few nits in the description: For things like __name__ or file paths, use backticks around the text. That'll make …

chipx86chipx86

You'll want to maintain two blank lines between top-level sections of the file (import groups and declarations being their own …

chipx86chipx86

Similarly, two blank lines between the logger = and the class definition.

chipx86chipx86

The other lines in this statement no longer align with the start of the parameter on the first line.

chipx86chipx86

Same with the other file, there should be two blank lines on either side of this declaration.

chipx86chipx86

Since this code is changing anyway, it'd be nice to move it to the modern KGB spy checks. We haven't …

chipx86chipx86

This could also be changed to assertSpyCalled(), like above.

chipx86chipx86

Looks like the parameters to this statement still aren't aligned. Once that's fixed, we'll be good to land! :)

chipx86chipx86
chipx86
  1. Congrats on your first change! :)

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

  3. Show all issues

    A few nits in the description:

    1. For things like __name__ or file paths, use backticks around the text. That'll make it stand out as code/literals.
    2. Descriptions should wrap to somewhere around 75 characters, to best fit in Git commit messages.
    3. 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.

  4. reviewboard/integrations/models.py (Diff revision 1)
     
     
     
     
    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.

  5. reviewboard/integrations/models.py (Diff revision 1)
     
     
     
     
    Show all issues

    Similarly, two blank lines between the logger = and the class definition.

  6. reviewboard/integrations/models.py (Diff revision 1)
     
     
     
     
    Show all issues

    The other lines in this statement no longer align with the start of the parameter on the first line.

  7. reviewboard/integrations/tests/test_configs.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    Same with the other file, there should be two blank lines on either side of this declaration.

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

  9. Show all issues

    This could also be changed to assertSpyCalled(), like above.

  10. 
      
jordanvandenbruel
chipx86
  1. 
      
  2. reviewboard/integrations/models.py (Diff revision 2)
     
     
     
     
    Show all issues

    Looks like the parameters to this statement still aren't aligned. Once that's fixed, we'll be good to land! :)

  3. 
      
jordanvandenbruel
jordanvandenbruel
Review request changed
Status:
Completed
Change Summary:
Pushed to django-3.2 (0efa050)