Skip to content

fix: handled spaces in variable names for crosstab (#305)#321

Merged
jim-smith merged 17 commits intomainfrom
305-bug-error-when-using-crosstab-when-variable-names-contain-spaces
Feb 2, 2026
Merged

fix: handled spaces in variable names for crosstab (#305)#321
jim-smith merged 17 commits intomainfrom
305-bug-error-when-using-crosstab-when-variable-names-contain-spaces

Conversation

@JessUWE
Copy link
Contributor

@JessUWE JessUWE commented Jan 30, 2026

  • Implemented add_backticks helper to wrap variable names containing spaces, fixing numexpr syntax errors
  • Updated get_queries to apply backticks to row and column identifiers
  • Removed obsolete string manipulation in crosstab_with_totals

-Implemented `add_backticks` helper to wrap variable names containing spaces, fixing `numexpr` syntax errors
-Updated `get_queries` to apply backticks to row and column identifiers
-Removed obsolete string manipulation in `crosstab_with_totals`
@JessUWE JessUWE linked an issue Jan 30, 2026 that may be closed by this pull request
@JessUWE JessUWE requested a review from rpreen January 30, 2026 11:40
@codecov
Copy link

codecov bot commented Jan 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.92%. Comparing base (df8b3b7) to head (deab03e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   99.76%   99.92%   +0.16%     
==========================================
  Files           9        9              
  Lines        1253     1269      +16     
==========================================
+ Hits         1250     1268      +18     
+ Misses          3        1       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rpreen
Copy link
Collaborator

rpreen commented Jan 30, 2026

Do we need a unit test to confirm this works and make sure nobody breaks it in the future?

It would be nice to have type hints in all the functions to help readability and help confirm behaviour with mypy, but I see lots of them missing all over the code base (not just here) - @jim-smith that might be a good first task for someone - along with lots of refactoring and test improvements?

JessUWE and others added 6 commits January 30, 2026 12:18
…add tests for add_backticks and rounded_survival_table
- Added import for rounded_survival_table at module level (fixes PLC0415)
- Added test_add_backticks() to test the backticks helper function
- Added test_rounded_survival_table() to test survival table rounding
- All tests pass with 99% coverage
Resolved merge conflict: kept top-level import for rounded_survival_table
and removed inline import from test function.
@jim-smith
Copy link
Contributor

@rpreen @JessUWE :

  1. Yes there should be a unit test for this (not a fan of pragma: nocoverage) -- but it looks like there is one so why do we need the pragma?
  2. Yes to adding typehints being a good job for an intern if we can get the right one

@jim-smith jim-smith self-requested a review January 30, 2026 17:36
@jim-smith
Copy link
Contributor

@JessUWE I've approve and merged in the PR that provides a test for the rounded_survival_table()
so you might want other merge the updated main back into this branch before you add the new test for cross tab() with variables that have spaces in?

@jim-smith
Copy link
Contributor

@JessUWE but the rest of the code is a sensible and better refactored handling of the issue this PR addresses

@JessUWE JessUWE requested a review from jim-smith January 30, 2026 19:36
@jim-smith jim-smith merged commit cda2d31 into main Feb 2, 2026
5 checks passed
@jim-smith jim-smith deleted the 305-bug-error-when-using-crosstab-when-variable-names-contain-spaces branch February 2, 2026 08:48
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.

[BUG] Error when using crosstab() when variable names contain spaces

3 participants