Use named modules for integration modules logger.

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

jordanvandenbruel
Review Board
release-4.0.x
4906
reviewboard

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 Author
Use named modules for integration modules logger.
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. "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. 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)
     
     
     
     

    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)
     
     
     
     

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

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

    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)
     
     
     
     
     
     

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

  8. 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. This could also be changed to assertSpyCalled(), like above.

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

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

  3. 
      
jordanvandenbruel
Review request changed

Commits:

Summary Author
-
Use named modules for integration modules logger.
Jordan
+
Use named modules for integration modules logger.
Jordan

Diff:

Revision 3 (+34 -22)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...