Copilot commented on code in PR #2303:
URL:
https://github.com/apache/incubator-pegasus/pull/2303#discussion_r2436541798
##########
python-client/pypegasus/pgclient.py:
##########
@@ -43,8 +44,15 @@
except:
fastbinary = None
+try:
+ with open(os.path.dirname(__file__)+"/logger.yaml", 'r', encoding='utf-8')
as f:
+ config = yaml.safe_load(f)
+ logging.config.dictConfig(config)
+ logging.getLogger("pgclient").info("Logging config loaded
successfully.")
Review Comment:
[nitpick] Emitting an INFO log on import can be noisy for applications.
Consider using debug level or removing this message to avoid unexpected logs
during import.
```suggestion
logging.getLogger("pgclient").debug("Logging config loaded
successfully.")
```
##########
python-client/pypegasus/logger.yaml:
##########
@@ -0,0 +1,24 @@
+version: 1
+disable_existing_loggers: false
+
+formatters:
Review Comment:
The removed logger.conf contained the ASF license header, but the new
logger.yaml has no header. Please add the standard ASF license header as YAML
comments at the top to maintain license compliance.
##########
python-client/pypegasus/pgclient.py:
##########
@@ -43,8 +44,15 @@
except:
fastbinary = None
+try:
+ with open(os.path.dirname(__file__)+"/logger.yaml", 'r', encoding='utf-8')
as f:
Review Comment:
[nitpick] Use os.path.join instead of string concatenation for
cross-platform path construction. For example: config_path =
os.path.join(os.path.dirname(__file__), 'logger.yaml'); with open(config_path,
'r', encoding='utf-8') as f: ...
```suggestion
with open(os.path.join(os.path.dirname(__file__), "logger.yaml"), 'r',
encoding='utf-8') as f:
```
##########
python-client/setup.py:
##########
@@ -23,7 +23,7 @@
version=pypegasus.__version__,
install_requires=['Twisted==21.2.0', 'aenum==3.0.0', 'thrift==0.13.0',
'pyOpenSSL==24.2.1','cryptography==43.0.1'],
Review Comment:
You import yaml in pgclient.py but PyYAML is not declared in
install_requires. This will raise ImportError at runtime for users who don't
already have PyYAML installed. Add a dependency such as 'PyYAML>=5.1' (or a
project-approved range).
```suggestion
install_requires=['Twisted==21.2.0', 'aenum==3.0.0', 'thrift==0.13.0',
'pyOpenSSL==24.2.1','cryptography==43.0.1', 'PyYAML>=5.1'],
```
##########
python-client/pypegasus/pgclient.py:
##########
@@ -43,8 +44,15 @@
except:
fastbinary = None
+try:
+ with open(os.path.dirname(__file__)+"/logger.yaml", 'r', encoding='utf-8')
as f:
+ config = yaml.safe_load(f)
+ logging.config.dictConfig(config)
+ logging.getLogger("pgclient").info("Logging config loaded
successfully.")
+except Exception as e:
+ print(f"Failed to load pgclient logging config: {e}")
+ logging.basicConfig(level=logging.INFO)
Review Comment:
[nitpick] Avoid printing directly from a library; prefer logging or
warnings. Consider initializing basicConfig first, then emit a warning via the
module logger, e.g., logging.basicConfig(level=logging.INFO);
logging.getLogger('pgclient').warning('Failed to load pgclient logging config:
%s', e).
```suggestion
logging.basicConfig(level=logging.INFO)
logging.getLogger("pgclient").warning("Failed to load pgclient logging
config: %s", e)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
