Skip to content

removed underscore logic from rules #1738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alexfurmenkov wants to merge 6 commits into
base: main
Choose a base branch
from
Open

removed underscore logic from rules #1738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c2da379
removed underscore logic from rules
alexfurmenkov May 18, 2026
075a44f
fix for tests
alexfurmenkov May 19, 2026
39484ad
fix for tests
alexfurmenkov May 20, 2026
c6c1190
Merge branch 'main' into 1732-remove-underscore
alexfurmenkov May 20, 2026
9d6439c
fix duplicated rule ids
alexfurmenkov May 20, 2026
b359e4b
Merge branch 'main' into 1732-remove-underscore
RamilCDISC May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 30 additions & 33 deletions cdisc_rules_engine/models/rule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,21 @@ def __init__(self, record_params: dict):
self.output_variables: dict = record_params.get("output_variables")
self.grouping_variables: List[str] = record_params.get("grouping_variables", [])

@classmethod
def _get_key(cls, obj: dict, space_key: str, default=None):
"""Get a value by key, supporting both space-format ('Rule Type') and
underscore-format ('Rule_Type') for backward compatibility with the
Copy link
Copy Markdown
Collaborator

@RamilCDISC RamilCDISC May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand having backward compatibility is good but the ticket AC is to remove underscore format, which was for rule editor. @alexfurmenkov @SFJohnson24 do you agree to keep backward compatibility or fully remove it? From the ticket AC we should remove this fully.

CDISC Library API which returns underscore keys."""
if space_key in obj:
return obj[space_key]
return obj.get(space_key.replace(" ", "_"), default)

@classmethod
def from_cdisc_metadata(cls, rule_metadata: dict) -> dict:
if cls.is_cdisc_rule_metadata(rule_metadata):
rule_metadata = cls.spaces_to_underscores(rule_metadata)
authorities = rule_metadata.get("Authorities", [])
scope = rule_metadata.get("Scope", {})
outcome = rule_metadata.get("Outcome", {})
executable_rule = {
"core_id": rule_metadata.get("Core", {}).get("Id"),
"author": "CDISC",
Expand All @@ -49,49 +59,36 @@ def from_cdisc_metadata(cls, rule_metadata: dict) -> dict:
"description": rule_metadata.get("Description"),
"authorities": authorities,
"standards": cls.parse_standards(authorities),
"classes": rule_metadata.get("Scope", {}).get("Classes"),
"domains": rule_metadata.get("Scope", {}).get("Domains"),
"entities": rule_metadata.get("Scope", {}).get("Entities"),
"rule_type": rule_metadata.get("Rule_Type"),
"classes": scope.get("Classes"),
"domains": scope.get("Domains"),
"entities": scope.get("Entities"),
"rule_type": cls._get_key(rule_metadata, "Rule Type"),
"conditions": cls.parse_conditions(rule_metadata.get("Check")),
"actions": cls.parse_actions(rule_metadata.get("Outcome")),
"use_case": rule_metadata.get("Scope", {}).get("Use_Case"),
"data_structures": rule_metadata.get("Scope", {}).get(
"Data_Structures"
),
"actions": cls.parse_actions(outcome),
"use_case": cls._get_key(scope, "Use Case"),
"data_structures": cls._get_key(scope, "Data Structures"),
"status": rule_metadata.get("Core", {}).get("Status", {}),
}

if "Operations" in rule_metadata:
executable_rule["operations"] = rule_metadata.get("Operations")

if "Match_Datasets" in rule_metadata:
executable_rule["datasets"] = cls.parse_datasets(
rule_metadata.get("Match_Datasets")
)
if "Output_Variables" in rule_metadata.get("Outcome", {}):
executable_rule["output_variables"] = rule_metadata.get("Outcome", {})[
"Output_Variables"
]
if "Grouping_Variables" in rule_metadata:
executable_rule["grouping_variables"] = rule_metadata.get(
"Grouping_Variables"
)
match_datasets = cls._get_key(rule_metadata, "Match Datasets")
if match_datasets is not None:
executable_rule["datasets"] = cls.parse_datasets(match_datasets)

output_variables = cls._get_key(outcome, "Output Variables")
if output_variables is not None:
executable_rule["output_variables"] = output_variables

grouping_variables = cls._get_key(rule_metadata, "Grouping Variables")
if grouping_variables is not None:
executable_rule["grouping_variables"] = grouping_variables

return executable_rule
else:
return rule_metadata

@classmethod
def spaces_to_underscores(cls, obj):
if isinstance(obj, dict):
return {
key.replace(" ", "_"): cls.spaces_to_underscores(value)
for key, value in obj.items()
}
if isinstance(obj, list):
return [cls.spaces_to_underscores(item) for item in obj]
return obj

@classmethod
def parse_standards(cls, authorities: List[dict]) -> List[dict]:
standards = []
Expand Down
23 changes: 12 additions & 11 deletions cdisc_rules_engine/models/rule_validation_result.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,18 @@ def from_skipped_rule(
return instance

def _get_rule_ids(self, rule: Rule, org: str) -> str:
return ", ".join(
sorted(
{
reference.get("Rule_Identifier", {}).get("Id")
for authority in rule.get("authorities", [])
for standard in authority.get("Standards", [])
for reference in standard.get("References", [])
if authority.get("Organization") == org
}
)
)
ids = {
(
reference.get("Rule Identifier")
or reference.get("Rule_Identifier")
or {}
).get("Id")
for authority in rule.get("authorities", [])
for standard in authority.get("Standards", [])
for reference in standard.get("References", [])
if authority.get("Organization") == org
}
return ", ".join(sorted(id_ for id_ in ids if id_ is not None))

def to_representation(self) -> dict:
return {
Expand Down
14 changes: 1 addition & 13 deletions scripts/script_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ def load_and_parse_rule(rule_file):
with open(rule_file, "r", encoding="utf-8") as file:
if file_extension in [".yml", ".yaml"]:
loaded_data = yaml.safe_load(file)
return Rule.from_cdisc_metadata(replace_yml_spaces(loaded_data))
return Rule.from_cdisc_metadata(loaded_data)
elif file_extension == ".json":
return Rule.from_cdisc_metadata(json.load(file))
else:
Expand Down Expand Up @@ -612,18 +612,6 @@ def get_max_dataset_size(dataset_paths: Iterable[str]):
return max_dataset_size


def replace_yml_spaces(data):
if isinstance(data, dict):
return {
key.replace(" ", "_"): replace_yml_spaces(value)
for key, value in data.items()
}
elif isinstance(data, list):
return [replace_yml_spaces(item) for item in data]
else:
return data


def library_metadata_not_found_message(standard, version, substandard=None):
version_display = (version or "").replace("-", ".")
sub_part = f" substandard {substandard}" if substandard else ""
Expand Down
24 changes: 12 additions & 12 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -938,14 +938,14 @@ def mock_validation_results() -> list[RuleValidationResult]:
"Standards": [
{
"References": [
{"Rule_Identifier": {"Id": "CDISCRuleID4"}},
{"Rule_Identifier": {"Id": "CDISCRuleID3"}},
{"Rule Identifier": {"Id": "CDISCRuleID4"}},
{"Rule Identifier": {"Id": "CDISCRuleID3"}},
]
},
{
"References": [
{"Rule_Identifier": {"Id": "CDISCRuleID2"}},
{"Rule_Identifier": {"Id": "CDISCRuleID1"}},
{"Rule Identifier": {"Id": "CDISCRuleID2"}},
{"Rule Identifier": {"Id": "CDISCRuleID1"}},
]
},
],
Expand All @@ -955,8 +955,8 @@ def mock_validation_results() -> list[RuleValidationResult]:
"Standards": [
{
"References": [
{"Rule_Identifier": {"Id": "FDARuleID1"}},
{"Rule_Identifier": {"Id": "FDARuleID2"}},
{"Rule Identifier": {"Id": "FDARuleID1"}},
{"Rule Identifier": {"Id": "FDARuleID2"}},
]
}
],
Expand Down Expand Up @@ -998,14 +998,14 @@ def mock_validation_results() -> list[RuleValidationResult]:
"Standards": [
{
"References": [
{"Rule_Identifier": {"Id": "CDISCRuleID4"}},
{"Rule_Identifier": {"Id": "CDISCRuleID3"}},
{"Rule Identifier": {"Id": "CDISCRuleID4"}},
{"Rule Identifier": {"Id": "CDISCRuleID3"}},
]
},
{
"References": [
{"Rule_Identifier": {"Id": "CDISCRuleID2"}},
{"Rule_Identifier": {"Id": "CDISCRuleID1"}},
{"Rule Identifier": {"Id": "CDISCRuleID2"}},
{"Rule Identifier": {"Id": "CDISCRuleID1"}},
]
},
],
Expand All @@ -1015,8 +1015,8 @@ def mock_validation_results() -> list[RuleValidationResult]:
"Standards": [
{
"References": [
{"Rule_Identifier": {"Id": "FDARuleID1"}},
{"Rule_Identifier": {"Id": "FDARuleID2"}},
{"Rule Identifier": {"Id": "FDARuleID1"}},
{"Rule Identifier": {"Id": "FDARuleID2"}},
]
}
],
Expand Down
18 changes: 6 additions & 12 deletions tests/resources/CoreIssue295/SEND4.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@
"Description": "Raise an error when the data type in the define.xml does not correspond to the data type in the dataset",
"Outcome": {
"Message": "Variable datatype in the define.xml does not correspond to the data type in the dataset",
"Output_Variables": [
"variable_name"
]
"Output Variables": ["variable_name"]
},
"Sensitivity": "Record",
"Executability": "Fully Executable",
Expand All @@ -45,7 +43,7 @@
"Cited_Guidance": "The define.xml specification includes seven distinct attributes to describe variable-level metadata: ..."
}
],
"Rule_Identifier": {
"Rule Identifier": {
"Id": "SEND4",
"Version": "1"
}
Expand All @@ -57,15 +55,11 @@
],
"Scope": {
"Classes": {
"Include": [
"ALL"
]
"Include": ["ALL"]
},
"Domains": {
"Include": [
"ALL"
]
"Include": ["ALL"]
}
},
"Rule_Type": "Variable Metadata Check against Define XML"
}
"Rule Type": "Variable Metadata Check against Define XML"
}
16 changes: 8 additions & 8 deletions tests/resources/Rule-CG0027.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"Description": "Trigger error when --SCAT is not null and --SCAT is equal to --CAT",
"Outcome": {
"Message": "--SCAT is equal to --CAT",
"Output_Variables": ["--CAT", "--SCAT"]
"Output Variables": ["--CAT", "--SCAT"]
},
"Sensitivity": "Record",
"Authorities": [
Expand All @@ -39,10 +39,10 @@
"Document": "Model v2.0",
"Item": "--SCAT",
"Section": "Interventions",
"Cited_Guidance": "A further grouping or classification of the category for the topic of the finding, event, or intervention. The category is in --CAT."
"Cited Guidance": "A further grouping or classification of the category for the topic of the finding, event, or intervention. The category is in --CAT."
}
],
"Rule_Identifier": {
"Rule Identifier": {
"Id": "CG0027",
"Version": "1"
}
Expand All @@ -61,10 +61,10 @@
"Document": "Model v1.7",
"Item": "--SCAT",
"Section": "2.2.1",
"Cited_Guidance": "Used to define a further categorization of --CAT values."
"Cited Guidance": "Used to define a further categorization of --CAT values."
}
],
"Rule_Identifier": {
"Rule Identifier": {
"Id": "CG0027",
"Version": "1"
}
Expand All @@ -83,10 +83,10 @@
"Document": "Model v1.4",
"Item": "Table 2.2.1, --SCAT",
"Section": "2.2.1",
"Cited_Guidance": "Used to define a further categorization of --CAT values."
"Cited Guidance": "Used to define a further categorization of --CAT values."
}
],
"Rule_Identifier": {
"Rule Identifier": {
"Id": "CG0027",
"Version": "1"
}
Expand All @@ -104,5 +104,5 @@
"Include": ["ALL"]
}
},
"Rule_Type": "Record Data"
"Rule Type": "Record Data"
}
Loading