Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use __name__ for loggers, per convention #989

Merged
merged 1 commit into from
Mar 11, 2020

Conversation

joshuagl
Copy link
Member

@joshuagl joshuagl commented Mar 3, 2020

Fixes issue #: N/A

Description of the changes being introduced by the pull request:

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

Replace hard-coded logger names with __name__. For the most part this just uses
the standard conventions to create the same logger hierarchy as existed before.
The only real difference is that loggers created for printing during tests are
no longer part of the 'tuf' hierarchy.

Signed-off-by: Joshua Lock <jlock@vmware.com>
@@ -113,6 +113,9 @@
file_handler = None

# Set the logger and its settings.
# Note: we're configuring the top-level hierarchy for the tuf package,
# therefore we explicitly request the 'tuf' logger, rather than following
# the standard pattern of logging.getLogger(__name__)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could avoid this if we move initialising the top-level logger into tuf/__init__.py, but as we will still want this module and its API to be able to configure the logger hierarchy it didn't seem particularly beneficial to make that change.

@lukpueh
Copy link
Member

lukpueh commented Mar 3, 2020

Hooray for passing AppVeyor tests (see #985). :)

Copy link
Member

@trishankatdatadog trishankatdatadog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this much less brittle code, Joshua! 🎉

# We explicitly create a logger which is a child of the tuf hierarchy,
# instead of using the standard getLogger(__name__) pattern, because the
# tests are not part of the tuf hierarchy and we are testing functionality
# of the tuf package explicitly enabled on the tuf hierarchy
logger = logging.getLogger('tuf.test_log')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger = logging.getLogger('tuf.test_log')
logger = logging.getLogger(__name__)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the comment above I explicitly didn't change to __name__ here because that gives us a logger names "test_log", which isn't part of the "tuf" logger hierarchy and therefore we can not test the logging functionality in tuf.log which only affects the "tuf" logger and its children.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, somehow missed that! Curious, is this necessary for the tests to work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is necessary for the tests to work.

Functions in tuf.log change logger configuration settings on the "tuf" logger and its children. Therefore if we create a logger in the test named "test_log" (via getLogger(__name__)) the tuf.log calls don't affect the logger we created (as it's not a member of the "tuf" logger hierarchy) and the tests fail:

======================================================================
FAIL: test_set_log_level (test_log.TestLog)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/joshuagl/Projects/tuf/tests/test_log.py", line 62, in test_set_log_level
    self.assertTrue(logger.isEnabledFor(logging.DEBUG))
AssertionError: False is not true

Copy link
Contributor

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Should make logging control in the context of Warehouse a good deal easier 😄

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Many thanks for taking on this dull but very valuable task! :)

@@ -220,7 +220,8 @@ def __init__(self, repository_directory, metadata_directory,
def writeall(self, consistent_snapshot=False):
"""
<Purpose>
Write all the JSON Metadata objects to their corresponding files.
Write all the JSON Metadata objects to their corresponding files for
roles which have changed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this slip in by accident? I'm okay with leaving it there. The rest of the PR is very clear and this is a legitimate clarification of writeall's purpose (see #958 and #964).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it did. Whoops.

@lukpueh lukpueh merged commit 256aef8 into theupdateframework:develop Mar 11, 2020
@joshuagl joshuagl deleted the logger branch March 11, 2020 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants