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 12 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
12 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
5adb42d
Merge branch 'main' into 1732-remove-underscore
SFJohnson24 May 27, 2026
d1a823b
slight tweak
SFJohnson24 May 27, 2026
8815d3a
added underscore
SFJohnson24 May 27, 2026
a8d7096
Merge branch 'main' into 1732-remove-underscore
gerrycampion Jun 16, 2026
ac8543e
additional underscore fixes
gerrycampion Jun 16, 2026
91e1669
Merge branch 'main' into 1732-remove-underscore
RamilCDISC Jun 18, 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
69 changes: 33 additions & 36 deletions cdisc_rules_engine/models/rule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,24 @@ def __init__(self, record_params: dict):
self.operations: List[dict] = record_params.get("operations")
self.conditions: dict = record_params["conditions"]
self.actions: dict = record_params["actions"]
self.output_variables: dict = record_params.get("output_variables")
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

@RamilCDISC RamilCDISC May 21, 2026

Copy link
Copy Markdown
Collaborator

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.

@SFJohnson24 SFJohnson24 May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RamilCDISC yes we would like to remove the underscore logic. This PR should be merged after the API changes https://github.com/cdisc-org/cdisc-library-api/issues/16 as it will no longer have underscores

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 Expand Up @@ -200,8 +197,8 @@ def parse_datasets(cls, match_key_data: List[dict]) -> List[dict]:
],
"wildcard": data.get("Wildcard", "**"),
}
if "Join_Type" in data:
join_data["join_type"] = data.get("Join_Type")
if "Join Type" in data:
join_data["join_type"] = data.get("Join Type")
if "Child" in data:
join_data["child"] = data.get("Child")
datasets.append(join_data)
Expand Down
7 changes: 6 additions & 1 deletion 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 @@ -48,12 +48,17 @@ def _get_rule_ids(self, rule: Rule, org: str) -> str:
return ", ".join(
sorted(
{
reference.get("Rule_Identifier", {}).get("Id")
(
reference.get("Rule Identifier")
or reference.get("Rule_Identifier")

@SFJohnson24 SFJohnson24 Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

note this line will be removed once github api logic is merged

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
}
- {None}
)
)

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 @@ -604,18 +604,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
8 changes: 4 additions & 4 deletions tests/resources/COREIssue329/rule-286.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
"Document": "IG v3.2",
"Item": "Assumption 3",
"Section": "7.4",
"Cited_Guidance": "Further information about the parameters is included below in Table 1. TSVAL may have controlled terminology depending on the value of TSPARMCD"
"Cited Guidance": "Further information about the parameters is included below in Table 1. TSVAL may have controlled terminology depending on the value of TSPARMCD"
}
],
"Rule_Identifier": {
"Rule Identifier": {
"Id": "CG0286",
"Version": "1"
}
Expand All @@ -37,10 +37,10 @@
"Document": "IG v3.3",
"Item": "Assumption 3",
"Section": "7.4",
"Cited_Guidance": "Further information about the parameters is included Appendix C1, Trial Summary Codes. TSVAL may have controlled terminology depending on the value of TSPARMCD. Conditions for including parameters are included in Appendix C1, Trial Summary Codes."
"Cited Guidance": "Further information about the parameters is included Appendix C1, Trial Summary Codes. TSVAL may have controlled terminology depending on the value of TSPARMCD. Conditions for including parameters are included in Appendix C1, Trial Summary Codes."
}
],
"Rule_Identifier": {
"Rule Identifier": {
"Id": "CG0286",
"Version": "1"
}
Expand Down
Loading
Loading