fix(stats): Only strip known global prefixes in strip_namespace - #718
fix(stats): Only strip known global prefixes in strip_namespace#718yiyche wants to merge 1 commit into
Conversation
Previously, the CSV importer blindly stripped everything before the first colon, which aggressively dropped custom namespaces (like oecd:). This updates strip_namespace and strip_namespace_series to strictly target known prefixes (dcid:, dcs:, schema:).
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the namespace stripping logic in simple/stats/data.py to only strip specific global prefixes (dcid:, dcs:, and schema:) instead of splitting on any colon, and adds corresponding unit tests. The reviewer suggests removing this prefix-stripping logic entirely, advising against adding defensive input-cleaning code for patterns that are guaranteed by the system's architecture to never occur.
| _DCS_PREFIX = "dcs:" | ||
|
|
||
| _NAMESPACE_DELIMITER = ':' | ||
| _GLOBAL_PREFIXES = ("dcid:", "dcs:", "schema:") |
There was a problem hiding this comment.
Avoid adding defensive code or input-cleaning logic (such as stripping prefixes) for states or patterns that are guaranteed by the system's architecture or design to never occur in the input data. We should remove this prefix stripping logic entirely.
References
- Avoid adding defensive code or input-cleaning logic (such as stripping prefixes) for states or patterns that are guaranteed by the system's architecture or design to never occur in the input data.
| provenance_ids: list[str] = field(default_factory=list) | ||
| source_ids: list[str] = field(default_factory=list) | ||
|
|
||
| def add_provenance(self, provenance: "Provenance") -> "StatVarGroup": |
There was a problem hiding this comment.
Does the call to the strip_namespace method need to be somewhere here as well?
2 participants
Previously, the CSV importer blindly stripped everything before the first colon, which dropped custom namespaces (like oecd:). This updates strip_namespace and strip_namespace_series to strictly target known prefixes (dcid:, dcs:, schema:).