Oct 4, 2022 · Nov 1, 2022 · Nov 1, 2022 · Aug 7, 2022 · Aug 25, 2022 · Nov 1, 2022
diff --git a/gitlab/__init__.py b/gitlab/__init__.py
 import warnings
 from typing import Any

 import gitlab._logging
 import gitlab.config  # noqa: F401
 from gitlab import utils as _utils
 from gitlab._version import (  # noqa: F401
diff --git a/gitlab/_logging.py b/gitlab/_logging.py
 # -*- 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/>.

 importlogging
 fromtypingimportList

 __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())
diff --git a/gitlab/client.py b/gitlab/client.py
 """Wrapper for the GitLab API."""

 import logging
 import os
 import re
 import time
 import gitlab.exceptions
 from gitlab import _backends, utils

 LOG = logging.getLogger(__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: "

    @staticmethod
    def enable_debug() -> None:
        import logging
        from http.client import HTTPConnection  # noqa

        HTTPConnection.debuglevel = 1
        requests_log = logging.getLogger("requests.packages.urllib3")
        requests_log.setLevel(logging.DEBUG)
        requests_log.propagate = True
        LOG.debug("Enabled debug mode for python-gitlab")

    def _get_session_opts(self) -> Dict[str, Any]:
        return {
diff --git a/tests/unit/test__logging.py b/tests/unit/test__logging.py
 importlogging

 importpytest

 fromgitlabimport_logging


 @pytest.fixture
 defLOG():
 returnlogging.getLogger(_logging._module_root_logger_name)


 deftest_module_root_logger_name():
 assert_logging._module_root_logger_name=="gitlab"


 deftest_module_name(LOG):
 assertLOG.name=="gitlab"


 deftest_logger_null_handler(LOG):
 assertlen(LOG.handlers)==1
 handler=LOG.handlers[0]
 assertisinstance(handler,logging.NullHandler)
Original file line number	Diff line number	Diff line change
Expand Up		@@ -19,6 +19,7 @@
		import warnings
		from typing import Any

		import gitlab._logging
		import gitlab.config # noqa: F401
		from gitlab import utils as _utils
		from gitlab._version import ( # noqa: F401
Expand Down
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/>.
Comment on lines +9 to +20 Copy link Member nejchNov 1, 2022 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. Copy link MemberAuthor JohnVillalovosNov 1, 2022 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 👍

		importlogging
		fromtypingimportList

		__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())
Copy link Member nejchAug 7, 2022 Choose a reason for hiding this comment The 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`__init__.py` a bit more - especially if this is extra module is being done just for testing.`requests` and a lot of others do that too just in`__init__.py`. Maybe we can look at other ways of testing if needed (or we can add more testing later)? Copy link MemberAuthor JohnVillalovosAug 25, 2022 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. Added a docstring to explain why doing it here. Copy link Member nejchNov 1, 2022 Choose a reason for hiding this comment The 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.`import _clients` would go before this. So I thought there might be a simple way for this, as I don't really see it done in popular python libraries. I know some use`get_logger()` helpers to lazily load loggers, maybe that's the idea here and I'm not getting it 😁 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 Copy link lmilbaumNov 2, 2022 Choose a reason for hiding this comment The 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. Copy link lmilbaumNov 3, 2022 Choose a reason for hiding this comment The 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? Copy link Member nejchNov 3, 2022 Choose a reason for hiding this comment The 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
		@@ -0,0 +1,24 @@
		importlogging

		importpytest

		fromgitlabimport_logging


		@pytest.fixture
		defLOG():
		returnlogging.getLogger(_logging._module_root_logger_name)


		deftest_module_root_logger_name():
		assert_logging._module_root_logger_name=="gitlab"


		deftest_module_name(LOG):
		assertLOG.name=="gitlab"
Comment on lines +8 to +18 Copy link Member nejchNov 1, 2022 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. Just a thought, but how about just doing a simple`logging.getLogger("gitlab")` in our code as that would make all the string manipulation and tests unnecessary. If the package name changes, users would have much bigger problems than this, as they would not be able to`import gitlab`.


		deftest_logger_null_handler(LOG):
		assertlen(LOG.handlers)==1
		handler=LOG.handlers[0]
		assertisinstance(handler,logging.NullHandler)