Test on all current Python versions #167
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR expands test coverage across newer Python versions, enhances data type inference for CSV inputs, and introduces slot range normalization in the RDFS importer.
- Update GitHub Actions matrix to include Python 3.11–3.13 and switch to
actions/cache@v3 - Enhance
infer_rangeto distinguishdatevsdatetimeand add_normalize_slot_rangesin the RDFS importer - Migrate Poetry dev dependencies to the new
group.devformat inpyproject.toml
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_importers/test_rdfs_importer.py | Added pytest import and updated expected slot count/checks |
| tests/test_generalizers/test_csv_data_generalizer.py | Added cases for date/datetime inference in infer_range tests |
| schema_automator/importers/rdfs_import_engine.py | Extended type hints, logging warnings on duplicate slots, and added slot range normalization |
| schema_automator/generalizers/csv_data_generalizer.py | Added date-only detection branch before datetime in infer_range |
| pyproject.toml | Converted dev-dependencies section to [tool.poetry.group.dev.dependencies] |
| .github/workflows/check-pull-request.yaml | Expanded matrix to Python 3.11–3.13, removed Windows, and allowed 3.13 to fail |
Comments suppressed due to low confidence (2)
.github/workflows/check-pull-request.yaml:22
- [nitpick] Since
windows-latesthas been removed from the matrix, the existing exclude block for Windows + 3.9 is now redundant; consider removing it to simplify the workflow.
exclude:
pyproject.toml:63
- Verify that your Poetry version supports the new
group.dev.dependenciestable; older versions expecttool.poetry.dev-dependenciesand may break CI.
[tool.poetry.group.dev.dependencies]
| @@ -5,6 +5,7 @@ | |||
| from io import StringIO | |||
| import unittest | |||
| import os | |||
| import pytest | |||
There was a problem hiding this comment.
The pytest import is never used in this test file; consider removing the unused import.
| import pytest |
Copilot uses AI. Check for mistakes.
| default_prefix: str | None = None, | ||
| model_uri: str | None = None, | ||
| identifier: str | None = None, | ||
| file: Union[str, Path, TextIO], |
There was a problem hiding this comment.
The parameter name file shadows a common built-in; consider renaming it (e.g., source or input_path) for better clarity.
| file: Union[str, Path, TextIO], | |
| input_path: Union[str, Path, TextIO], |
Copilot uses AI. Check for mistakes.
| identifier: str | None = None, | ||
| file: Union[str, Path, TextIO], | ||
| name: Optional[str] = None, | ||
| format: Optional[str] = "turtle", |
There was a problem hiding this comment.
The parameter name format shadows the built-in format function; consider renaming it (e.g., rdf_format) to avoid confusion.
| format: Optional[str] = "turtle", | |
| rdf_format: Optional[str] = "turtle", |
Copilot uses AI. Check for mistakes.
| if all( | ||
| not hasattr(parse(str(v)), 'hour') or | ||
| (parse(str(v)).hour == 0 and parse(str(v)).minute == 0 and parse(str(v)).second == 0) | ||
| for v in nn_vals |
There was a problem hiding this comment.
[nitpick] You call parse(str(v)) twice for each value in the date-only check; consider parsing once per value and reusing the result to eliminate redundant work.
| if all( | |
| not hasattr(parse(str(v)), 'hour') or | |
| (parse(str(v)).hour == 0 and parse(str(v)).minute == 0 and parse(str(v)).second == 0) | |
| for v in nn_vals | |
| parsed_vals = [parse(str(v)) for v in nn_vals] | |
| if all( | |
| not hasattr(pv, 'hour') or | |
| (pv.hour == 0 and pv.minute == 0 and pv.second == 0) | |
| for pv in parsed_vals |
Copilot uses AI. Check for mistakes.
Let's try to get ahead of things and run tests on the other Python versions to make sure we're still passing.