refactor: code cleanup, dead code removal, and linting improvements#31
refactor: code cleanup, dead code removal, and linting improvements#31jarsarasty merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies several internal/public method signatures by removing unused **kwargs parameters and marking unused placeholders with a leading underscore, and it cleans up unused constants.
Changes:
- Remove
**kwargsfrom exporter and adapter method signatures where not used. - Rename unused parameters (e.g.,
xml_time_series_dict→_xml_time_series_dict,mesido_data→_mesido_data) and update tests accordingly. - Remove unused constants from
common/constants.py.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| unit_test/test_esdl_adapter.py | Updates tests to match the _create_asset_from_esdl signature change. |
| src/kpicalculator/reporting/esdl_kpi_exporter.py | Removes unused **kwargs/typing import from export() signature. |
| src/kpicalculator/reporting/base_exporter.py | Removes **kwargs from the abstract export() signature. |
| src/kpicalculator/kpi_manager.py | Renames unused mesido parameter to underscore-prefixed placeholder. |
| src/kpicalculator/common/constants.py | Deletes unused constants. |
| src/kpicalculator/adapters/simulator_adapter.py | Removes unused **kwargs from load_data(). |
| src/kpicalculator/adapters/esdl_adapter.py | Renames unused _create_asset_from_esdl parameter to underscore-prefixed name. |
| src/kpicalculator/adapters/base_adapter.py | Narrows BaseAdapter.load_data() to only require source. |
Comments suppressed due to low confidence (1)
src/kpicalculator/adapters/esdl_adapter.py:356
- The parameter was renamed to
_xml_time_series_dict, but the docstringArgs:section still documentsxml_time_series_dict. Update the docstring to match the new parameter name (and, if appropriate, note that it's intentionally unused/legacy).
_xml_time_series_dict: PiXmlTimeSeries | None,
model_name: str,
) -> Asset | None:
"""Create an Asset object from an ESDL element.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d7db4fd to
7eefe3b
Compare
815bae9 to
ce51c09
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Remove dead code (time_series_protocols.py, unused constants)
- Extract shared helpers to eliminate duplication (BaseAdapter._parse_esdl_string,
DatabaseTimeSeriesLoader._process_single_profile,
TimeSeriesManager._load_xml_with_parameters/_load_xml_fallback)
- Reduce cyclomatic complexity in EsdlAdapter._process_energy_system
- Add ruff C9 (McCabe complexity) and ARG rules; refine per-file ignores
- Add pylint to analysis dependency group
- Rename run.py → check.py with optional --full pylint analysis
- Rename KpiManager.get_esdl_with_kpis() → build_esdl_with_kpis()
- Update architecture and implementation docs to reflect all changes
ce51c09 to
2bd14a0
Compare
MichielTukker
left a comment
There was a problem hiding this comment.
Maybe add version constraints in pyproject.toml
| analysis_rc |= _run( | ||
| [ | ||
| "uv", | ||
| "run", | ||
| "pylint", | ||
| "src/", | ||
| "--disable=all", | ||
| "--enable=similarities,R0914,R0912,R0915", | ||
| "--min-similarity-lines=6", | ||
| "--max-locals=16", | ||
| # xml_time_series_adapter.py is testing-only and excluded from all linting | ||
| "--ignore=xml_time_series_adapter.py", | ||
| ], | ||
| header="pylint — code analysis (duplicates, complexity, locals)", | ||
| ) |
There was a problem hiding this comment.
Why run pylint when the pyproject.toml contains a config for ruff?
There was a problem hiding this comment.
Ruff handles most of the linting and style checks. However, it doesn't cover duplicate code detection or
too-many-locals/too-many-branches. The pylint invocation here is scoped to exactly those checks
(--enable=similarities,R0914,R0912,R0915), so it's complementary rather than redundant. It's also only run with check.py --full, so it doesn't affect the default pre-commit flow.
I think this comment may belong to the security updates PR (#32).. I will add the version constraints there. |
DatabaseTimeSeriesLoader._process_single_profile,
TimeSeriesManager._load_xml_with_parameters/_load_xml_fallback)