94 replace print with logging - #95
Merged
Merged
Conversation
Contributor
Reviewer's GuideReplace ad-hoc print-based messaging with structured logging across core modules and tests, introducing a central logging setup and updating documentation to reflect the new logging conventions. Sequence diagram for CLI run with logging configurationsequenceDiagram
actor User
participant workflow_main as workflow.main
participant utils_setup_logging as utils.setup_logging
participant logger_workflow as workflow.logger
participant logger_statistics as statistics_functions.logger
participant logging_system as logging
User ->> workflow_main: main()
workflow_main ->> workflow_main: build_parser()
workflow_main ->> workflow_main: parse_args()
workflow_main ->> utils_setup_logging: setup_logging(level=args.log_level)
alt args.log_level == "DEBUG"
workflow_main ->> logger_workflow: logger.info(...)
end
workflow_main ->> workflow_main: resolve_config_path(config)
workflow_main ->> logging_system: logger.warning(...) [no config provided]
workflow_main ->> logger_statistics: logger.debug(...)
logger_statistics ->> logging_system: logger.debug(...)
logging_system -->> User: log output (configured level and format)
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
build_parser(), the--log-levelargument defaults to"DEBUG"while the help text says it defaults to WARNING; align the default and the help string to avoid confusion. - Several logging calls (e.g. the
logger.debugcalls instatistics_functions.py) use f-strings; consider switching to%-style formatting with arguments to avoid unnecessary string construction when the log level is disabled. - The
logger.info(target_unit)call in_get_unit_from_common_definitionslogs a bare value without context; include a descriptive message so that log output is self-explanatory.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `build_parser()`, the `--log-level` argument defaults to `"DEBUG"` while the help text says it defaults to WARNING; align the default and the help string to avoid confusion.
- Several logging calls (e.g. the `logger.debug` calls in `statistics_functions.py`) use f-strings; consider switching to `%`-style formatting with arguments to avoid unnecessary string construction when the log level is disabled.
- The `logger.info(target_unit)` call in `_get_unit_from_common_definitions` logs a bare value without context; include a descriptive message so that log output is self-explanatory.
## Individual Comments
### Comment 1
<location path="pypsa_validation_processing/workflow.py" line_range="54-55" />
<code_context>
default=None,
help="Path to YAML config file. Defaults to packaged config.",
)
+ parser.add_argument(
+ "--log-level",
+ default="DEBUG",
+ choices=["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"],
</code_context>
<issue_to_address>
**issue:** CLI log-level default and help text are inconsistent and may surprise users.
The default is set to `DEBUG`, but the help text says it defaults to `WARNING`, which is inconsistent and may surprise users given the new debug logging. Please align the actual default with the help text (e.g., change the default to `WARNING` to match the docstring and keep debug logs opt‑in, or update the help text if `DEBUG` is intended).
</issue_to_address>
### Comment 2
<location path="tests/test_format_timestamps.py" line_range="93-96" />
<code_context>
-def test_format_timestamps_sets_nat_on_localization_failure(capsys):
+def test_format_timestamps_sets_nat_on_localization_failure(caplog):
class FakeTimestamp:
tz = None
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also asserting on the second warning about columns set to NaT
This test currently asserts only the warning for failed localization. To fully validate the logging behavior described in the docstring, please also assert that a WARNING containing "columns set to NaT" appears in `caplog.records`.
Suggested implementation:
```python
def test_format_timestamps_sets_nat_on_localization_failure(caplog):
class FakeTimestamp:
tz = None
tzinfo = None
df = pd.DataFrame([[1.0]], columns=["2050-01-01 00:00:00"])
with caplog.at_level(logging.WARNING):
format_timestamps(df)
warning_messages = [
record.getMessage()
for record in caplog.records
if record.levelno == logging.WARNING
]
# Assert the original localization failure warning is logged
assert any("localization" in msg.lower() for msg in warning_messages)
# Assert the additional warning about columns set to NaT is also logged
assert any("columns set to NaT" in msg for msg in warning_messages)
```
If the existing test already contains assertions on the localization failure warning, you may want to:
1. Keep those assertions and only add the new `warning_messages` construction and the `assert any("columns set to NaT" ...)` line, rather than replacing the whole function body.
2. Adjust the string used in the localization warning assertion (`"localization" in msg.lower()`) to match the exact wording used by `format_timestamps`, or remove that assertion if it duplicates an existing one.
</issue_to_address>
### Comment 3
<location path="README.md" line_range="189" />
<code_context>
- Create a new branch linked to the respective issue
- Write your pypsa-statistics and add it as a separate function to [statistics_functions.py](https://github.com/maxnutz/pypsa_validation_processing/blob/main/pypsa_validation_processing/statistics_functions.py) (please note the [naming and structural conventions](https://github.com/maxnutz/pypsa_validation_processing/tree/main#variables-statistics---functions)!)
-- add a comprehensive docstring to your function
+- add a comprehensive docstring to your function and implement loggings
- add the mapping variable_name <> function_name to [mapping.default.yaml](https://github.com/maxnutz/pypsa_validation_processing/blob/main/pypsa_validation_processing/configs/mapping.default.yaml) (and your personal mapping-file)
- Add a testing routine for your Function to `tests/` - stick to the [testing-README](https://github.com/maxnutz/pypsa_validation_processing/blob/main/tests/README.md)
</code_context>
<issue_to_address>
**issue (typo):** Use singular "logging" instead of "loggings".
"implement loggings" is awkward usage; consider "implement logging" or "add appropriate logging" for clearer wording.
```suggestion
- add a comprehensive docstring to your function and add appropriate logging
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short description of this pull request
Implement a logging structure to replace the prints in all files. Adapt the tests on printing warnings etc.
Checklist
Before asking for review, please make shure, the following steps are completed (whenever possible):
Sourcery-Bot starts to review your pull request, whenever it is created. This may take some time. After having finished these steps, please request for review in the Pull Request.
Summary by Sourcery
Introduce a centralized logging strategy across the package and update the CLI and tests to use it instead of print-based messaging.
New Features:
Enhancements:
Documentation:
Tests: