Skip to content

make library_column_order a shared parent class as a move toward consistency#1671

Merged
gerrycampion merged 6 commits intomainfrom
filtered-variables
Mar 23, 2026
Merged

make library_column_order a shared parent class as a move toward consistency#1671
gerrycampion merged 6 commits intomainfrom
filtered-variables

Conversation

@gerrycampion
Copy link
Copy Markdown
Collaborator

@gerrycampion gerrycampion commented Mar 19, 2026

made library_column_order a shared parent class for expected_variables, permissible_variables, required_variables, and get_dataset_filtered_variables. Achieved by adding optional filter params for library_column_order.
This is to reduce inconsistent handling of variable_metadata and wildcard replacement.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Updated schema has not been merged with markdown descriptions. Please run the "Merge Schema with Markdown Descriptions" workflow to update the merged schema files.

@gerrycampion gerrycampion changed the title add filtered_variables shared class as a move toward consistency make library_column_order a shared parent class as a move toward consistency Mar 19, 2026
variables_metadata = [
var
for var in variables_metadata
if var.get(self.params.key_name) == self.params.key_value
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.

Previously for here is core was missing the code would fall back to "PERMISSIBLE" but here it would become None. Is this behavior change intended?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is the reason for the updates in sdtm_utilities. Does that resolve your concern?

return list(OrderedDict.fromkeys(variable_names_list))
# Filter variables based on the specified criteria

if self.params.key_name and self.params.key_value:
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.

This might not be a big issue for our case but if any chance key-value be 0 or False then this comparison will not work as intended. Could the better approach be: if self.params.key_name is not None and self.params.key_value: is not None ?

If you think these values are not possible here we can skip the change.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The values can be an empty string. I changed to only check truthiness of key_name. This way, if key_name is a non-empty string, we can do the filter. If key_value is missing, it will be treated as a filter for empty string.

@gerrycampion gerrycampion self-assigned this Mar 20, 2026
Copy link
Copy Markdown
Collaborator

@RamilCDISC RamilCDISC left a comment

Choose a reason for hiding this comment

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

The PR refactors the code to put shared code in parent class "library_column_order" to make code more consistent.
The PR was validated by:

  1. Review the PR for any unwanted code or comments.
  2. Review the PR in accordance with AC.
  3. Ensure all unit and regression testing pass.
  4. Ensure all relevant testing is updated.
  5. Ensure CORE_test_suite pass as this validates no rules are affected
  6. Manually run validation using dev editor.
  7. Ensure all downstream files are updated to use parent class.

@gerrycampion gerrycampion merged commit 16c89b1 into main Mar 23, 2026
13 checks passed
@gerrycampion gerrycampion deleted the filtered-variables branch March 23, 2026 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants