Fix date range without times #161
Conversation
|
Fixes #160 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue of improperly assigning date ranges without times and improves schema slot range normalization. Key changes include updating tests for date and datetime inference, adding duplicate slot warnings and normalization in the RDF import engine, and adjusting dependency and GitHub Action configurations.
- Updated test expectations in the RDFS importer and CSV data generalizer tests.
- Enhanced type annotations and slot range normalization in the RDF import engine.
- Upgraded configuration in pyproject.toml and GitHub workflows.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_importers/test_rdfs_importer.py | Updated expected slot count and added assertions for new slot names. |
| tests/test_generalizers/test_csv_data_generalizer.py | Added new test cases for date and datetime inference. |
| schema_automator/importers/rdfs_import_engine.py | Refactored type annotations, added warnings for duplicate slots, and introduced slot range normalization. |
| schema_automator/generalizers/csv_data_generalizer.py | Extended range inference with a check for dates without times. |
| pyproject.toml | Changed development dependency grouping. |
| .github/workflows/check-pull-request.yaml | Upgraded the caching action version from v2 to v3. |
| @@ -130,7 +131,10 @@ def convert( | |||
| cls_slots = defaultdict(list) | |||
|
|
|||
| for slot in self.generate_rdfs_properties(g, cls_slots): | |||
| sb.add_slot(slot) | |||
| if slot.name in sb.schema.slots: | |||
| warnings.warn(f"Slot '{slot.name}' already exists in schema; skipping duplicate.") | |||
There was a problem hiding this comment.
[nitpick] Consider reviewing the use of warnings.warn for duplicate slot names, as this may result in log flooding in production; using a logging mechanism instead might be preferable for better production diagnostics.
| warnings.warn(f"Slot '{slot.name}' already exists in schema; skipping duplicate.") | |
| logging.warning(f"Slot '{slot.name}' already exists in schema; skipping duplicate.") |
Copilot uses AI. Check for mistakes.
| @@ -645,6 +645,8 @@ def infer_range(slot: dict, vals: set, types: dict, coerce=True) -> str: | |||
| if all(isfloat(v) for v in nn_vals): | |||
| return 'float' | |||
| if all(is_date(v) for v in nn_vals): | |||
| if all(len(str(v).split('T')) == 1 for v in nn_vals): # Check if values are just dates without time | |||
There was a problem hiding this comment.
[nitpick] The current approach uses string splitting to differentiate between date and datetime; consider using a more robust time component check possibly leveraging date parsing results for more reliable inference.
| if all(len(str(v).split('T')) == 1 for v in nn_vals): # Check if values are just dates without time | |
| 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 | |
| ): # Check if values are just dates without time |
Copilot uses AI. Check for mistakes.
As shown in #160 , we don't properly assign the
daterange when a field doesn't include time. This should fix that and add a test to ensure it is properly assigned.