-
Notifications
You must be signed in to change notification settings - Fork 669
chore: add bare-minimum logging support #2204
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# -*- coding: utf-8 -*- | ||
""" | ||
The initialization of the logging module is intentionaly done here in the | ||
_logging.py module so that it will be initialized before any other of our | ||
python-gitlab modules are loaded. So if they do logging upon being loaded it | ||
will allow them to do it safely. | ||
""" | ||
|
||
# This program is free software: you can redistribute it and/or modify | ||
# it under the terms of the GNU Lesser General Public License as published by | ||
# the Free Software Foundation, either version 3 of the License, or | ||
# (at your option) any later version. | ||
# | ||
# This program is distributed in the hope that it will be useful, | ||
# but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
# GNU Lesser General Public License for more details. | ||
# | ||
# You should have received a copy of the GNU Lesser General Public License | ||
# along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
import logging | ||
from typing import List | ||
|
||
__all__: List[str] = [] | ||
|
||
# Using the `NullHandler` means that any log messages generated will not be | ||
# output unless the client application configures logging. For example by | ||
# calling `logging.basicConfig()` | ||
_module_root_logger_name = __name__.split(".", maxsplit=1)[0] | ||
logging.getLogger(_module_root_logger_name).addHandler(logging.NullHandler()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mght be misremembering but thought I saw an earlier version of this PR where I liked the simplicity of just doing this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a docstring to explain why doing it here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To me it just shifts the issue to imports if something happens to be imported e.g. I probably don't exactly understand the risks here for our own codebase or how you stumbled across this issue, so it would just be completely subjective from my side, maybe @max-wittig or @lmilbaum would be better to take a look here :P There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nejch Not sure I am following the discussion. Will take a deeper look at it in few hours. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've done some reading to figure out what the underscore means in the file name and its impact on time those lines of code are executed. Would you mind helping me figure this out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's to indicate a private module, and it is imported here to ensure the code is evaluated before any other logging code. But I'll let John explain this :) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,6 @@ | ||||||
"""Wrapper for the GitLab API.""" | ||||||
|
||||||
import logging | ||||||
import os | ||||||
import re | ||||||
import time | ||||||
|
@@ -14,6 +15,8 @@ | |||||
import gitlab.exceptions | ||||||
from gitlab import _backends, utils | ||||||
|
||||||
LOG = logging.getLogger(__name__) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can just use
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it is a global I prefer the Plus it is the way OpenStack does it: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As an example it is easy to do:
Which can cause problems. As I unfortunately know from my own experience. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, it was just an idea as it is a Logger instance that we manipulate and not a constant as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of the name |
||||||
|
||||||
REDIRECT_MSG = ( | ||||||
"python-gitlab detected a {status_code} ({reason!r}) redirection. You must update " | ||||||
"your GitLab URL to the correct URL to avoid issues. The redirection was from: " | ||||||
|
@@ -540,7 +543,6 @@ def _set_auth_info(self) -> None: | |||||
|
||||||
@staticmethod | ||||||
def enable_debug() -> None: | ||||||
import logging | ||||||
from http.client import HTTPConnection # noqa | ||||||
|
||||||
HTTPConnection.debuglevel = 1 | ||||||
|
@@ -549,6 +551,7 @@ def enable_debug() -> None: | |||||
requests_log = logging.getLogger("requests.packages.urllib3") | ||||||
requests_log.setLevel(logging.DEBUG) | ||||||
requests_log.propagate = True | ||||||
LOG.debug("Enabled debug mode for python-gitlab") | ||||||
nejch marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
def _get_session_opts(self) -> Dict[str, Any]: | ||||||
return { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import logging | ||
|
||
import pytest | ||
|
||
from gitlab import _logging | ||
|
||
|
||
@pytest.fixture | ||
def LOG(): | ||
return logging.getLogger(_logging._module_root_logger_name) | ||
|
||
|
||
def test_module_root_logger_name(): | ||
assert _logging._module_root_logger_name == "gitlab" | ||
|
||
|
||
def test_module_name(LOG): | ||
assert LOG.name == "gitlab" | ||
Comment on lines
+8
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a thought, but how about just doing a simple If the package name changes, users would have much bigger problems than this, as they would not be able to |
||
|
||
|
||
def test_logger_null_handler(LOG): | ||
assert len(LOG.handlers) == 1 | ||
handler = LOG.handlers[0] | ||
assert isinstance(handler, logging.NullHandler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this PR was started before we centralized the licensing into one file only, so maybe can be removed? Or if you feel strongly about license per file maybe just an SPDX header to keep it concise and machine-readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll get rid of that 👍