Skip to content

refactor: introduce ensure_value_choice() helper for enum-like string parameter validation #429

@murray-ds

Description

@murray-ds

Problem

The codebase has 14+ locations that independently validate string parameters against a fixed set of allowed values (sort_order, agg_func, orientation, data_label_format, etc.). This leads to:

  • Active bugscomposite_rank.py validates sort_order and agg_func with .lower() but uses the raw value in branching/lookup logic. sort_order="ASC" passes validation but silently produces descending ranks. agg_func="MEAN" passes validation but raises a KeyError on dict lookup.
  • Inconsistent error messages across modules
  • 3-5 lines of boilerplate duplicated at each call site

Bug example (composite_rank.py:260-264)

if sort_order.lower() not in valid_sort_orders:  # "ASC" passes validation
    ...
order_by = ibis.asc(df[col_name]) if sort_order in ["asc", "ascending"] else ibis.desc(df[col_name])
# "ASC" falls through to else → silently returns descending order

Proposal

Add ensure_value_choice() to pyretailscience/core/validation.py (alongside ensure_columns() from #428). A single function that validates a string value against allowed choices, optionally normalizing case, and returns the validated value.

Call Sites

Bugs (validate with .lower(), use raw value)

File Parameter Lines
analysis/composite_rank.py sort_order 260-264
analysis/composite_rank.py agg_func 351-355

Normalize-first (correct but duplicated)

File Parameter Lines
plots/waterfall.py data_label_format 86-94
plots/index.py agg_func 392-394

Exact match (correct but boilerplate)

File Parameter Lines
plots/bar.py orientation, sort_order, data_label_format 102-117
plots/index.py sort_by, sort_order 248-253
plots/histogram.py range_method 183-185
segmentation/segstats.py orientation, sort_order 616-619
analysis/cohort.py period 73-75

Acceptance Criteria

  • ensure_value_choice() exists in pyretailscience/core/validation.py
  • Full test coverage for the helper
  • Both composite_rank.py bugs are fixed
  • All call sites above are migrated
  • All existing tests pass
  • Error messages are consistent across the codebase after migration

Metadata

Metadata

Assignees

No one assigned

    Labels

    size:mediumMedium effort (2-5 days)status:readyFully specified, ready to be picked uptype:enhancementImprovement to existing functionality

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions