Updated Accounts module to use named logging.

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

mxwang
Review Board
release-4.0.x
4898
reviewboard

Only some logging statements in the Accounts module used named logging, when all of them should. So all logging statements were given the name "logger", and that was used instead.

Ex.
logging.warning("Some warning message")
becomes
logger = logging.getLogger(__name__) logger.warning("This is a warning to a named logger")

Python unit testing done.

Summary Author
Updated Accounts module to use named logging.
mxiuwang
Description From Last Updated

The description just repeats the summary. It helps to have a clear description of the purpose of the change. I ...

chipx86chipx86

The failed tests are due to the setup issues with cffi and such. They're good for us to know, but ...

chipx86chipx86

Is there a bug number for this? If so, can you put it in the "Bugs" field?

chipx86chipx86

The description needs to wrap at around 75 characters. This is important for ensuring that the text can properly fit ...

chipx86chipx86

In the top level of modules, we use 2 blank lines between sections. Imports are one section. Variable declarations are ...

chipx86chipx86

The parameters on the second line no longer align with the parameter on the first line, since logger is 1 ...

chipx86chipx86

This statement still has alignment errors.

chipx86chipx86

There's an alignment error between these lines.

chipx86chipx86

Since this module is now using named loggers, "X509Backend:" is no longer needed. That'll be covered by the logger name. ...

chipx86chipx86

This statement still has alignment issues.

chipx86chipx86

This statement still has some alignment problems for the parameters.

chipx86chipx86
chipx86
  1. 
      
  2. The description just repeats the summary. It helps to have a clear description of the purpose of the change. I like to follow the problem/solution format, describing the problem before (log entries from this module didn't include the module name, so it was more difficult to know the context) and then describing the solution (named loggers).

  3. The failed tests are due to the setup issues with cffi and such. They're good for us to know, but not good history to keep around, since it's not related to the change. You can remove those.

  4. Is there a bug number for this? If so, can you put it in the "Bugs" field?

  5. reviewboard/accounts/managers.py (Diff revision 1)
     
     
     
     
     
     

    In the top level of modules, we use 2 blank lines between sections. Imports are one section. Variable declarations are another. Classes are another.

    Just as there were two blank lines between the imports and the class, you'll want to two blank lines before and after logger = ....

    This will apply to the other modules as well.

  6. reviewboard/accounts/managers.py (Diff revision 1)
     
     
     

    The parameters on the second line no longer align with the parameter on the first line, since logger is 1 character shorter than logging. Can you update these, and everywhere else that this change was made?

  7. 
      
mxwang
Review request changed

Change Summary:

Fixed comments

Description:

~  

Updated Accounts module to use named logging.

  ~

Only some logging statements in the Accounts module used named logging, when all of them should. So all logging statements were given the name "logger", and that was used instead.

  +
  +

Ex.

  + logging.warning("Some warning message")
  + becomes
  + logger = logging.getLogger(__name__)
  + logger.warning("This is a warning to a named logger")

Testing Done:

~  

Python unit testing done. 3 tests failed, but they failed before the changes were made.

  ~

Python unit testing done.

-   Failed tests:
-   * FAIL: Testing RepositoryForm with a plain repository and SCMTool.prefers_mirror_path=True
-   * FAIL: Testing RepositoryForm with a plain repository and trusting SSL certificate with SCMTool.prefers_mirror_path
-   * FAIL: Testing the POST repositories/ API with all available info

Commits:

Summary Author
-
Updated Accounts module to use named logging.
mxiuwang
+
Updated Accounts module to use named logging.
mxiuwang

Bugs:

+4898

Diff:

Revision 2 (+108 -66)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
chipx86
  1. 
      
  2. The description needs to wrap at around 75 characters. This is important for ensuring that the text can properly fit in a Git commit.

  3. reviewboard/accounts/backends/registry.py (Diff revision 2)
     
     
     
     

    This statement still has alignment errors.

  4. reviewboard/accounts/backends/standard.py (Diff revision 2)
     
     
     

    There's an alignment error between these lines.

  5. reviewboard/accounts/backends/x509.py (Diff revision 2)
     
     
     
     
     
     

    Since this module is now using named loggers, "X509Backend:" is no longer needed. That'll be covered by the logger name. Can you remove these prefixes?

  6. reviewboard/accounts/forms/pages.py (Diff revision 2)
     
     
     
     

    This statement still has alignment issues.

  7. reviewboard/accounts/views.py (Diff revision 2)
     
     
     
     

    This statement still has some alignment problems for the parameters.

  8. 
      
Loading...