Skip to content

[PM-26292] tool: Add script to delete unused Localizable.strings entries#2464

Open
KatherineInCode wants to merge 8 commits intomainfrom
pm-26292/delete-unused-strings
Open

[PM-26292] tool: Add script to delete unused Localizable.strings entries#2464
KatherineInCode wants to merge 8 commits intomainfrom
pm-26292/delete-unused-strings

Conversation

@KatherineInCode
Copy link
Copy Markdown
Contributor

@KatherineInCode KatherineInCode commented Mar 18, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-26292

📔 Objective

This builds on #2412 by adding a module that finds unused strings and deletes them. I also did some refactoring of the strings file management into a separate python module.

I also ran the script, and it found 308 unused keys, many of which were identified in #2003. I'm not entirely sure why it's so many, especially of what seem like basic strings, though my guess is that some are used on Android, and a lot of them are detritus from MAUI or earlier that just never got cleaned up.

As noted in #2412, eventually we could have running these scripts be part of the CI process in some way, but currently these are still manual runs.

@github-actions github-actions bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context t:ci Change Type - Updates to automated workflows labels Mar 18, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 18, 2026 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – ca028752-3d1f-4b3c-9c00-23ec1cd22d2f

Great job! No new security vulnerabilities introduced in this pull request

@KatherineInCode KatherineInCode added the ai-review Request a Claude code review label Mar 18, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 18, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.96%. Comparing base (17aeced) to head (3d4fb78).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2464      +/-   ##
==========================================
- Coverage   87.09%   85.96%   -1.13%     
==========================================
  Files        1874     2104     +230     
  Lines      165816   180624   +14808     
==========================================
+ Hits       144411   155272   +10861     
- Misses      21405    25352    +3947

☔ 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.

@KatherineInCode KatherineInCode mentioned this pull request Mar 18, 2026
Merged
@KatherineInCode KatherineInCode changed the title [PM-26292] Add script to delete unused Localizable.strings entries [PM-26292] tool: Add script to delete unused Localizable.strings entries Mar 19, 2026
@KatherineInCode KatherineInCode added the innovation Feature work related to Innovation Sprint or BEEEP projects label Mar 31, 2026
Base automatically changed from pm-26293/delete-unused-strings to main April 9, 2026 14:15
@KatherineInCode KatherineInCode force-pushed the pm-26292/delete-unused-strings branch from f25e6b7 to 1b5766f Compare April 9, 2026 14:32
@KatherineInCode KatherineInCode marked this pull request as ready for review April 9, 2026 14:32
@KatherineInCode KatherineInCode requested review from a team and matt-livefront as code owners April 9, 2026 14:32
@KatherineInCode KatherineInCode force-pushed the pm-26292/delete-unused-strings branch from 1b5766f to 688112f Compare April 9, 2026 15:04
@KatherineInCode KatherineInCode force-pushed the pm-26292/delete-unused-strings branch from 688112f to 87d355a Compare April 9, 2026 15:10
@github-actions github-actions bot added t:feature and removed app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Apr 9, 2026
@github-actions github-actions bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Apr 9, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026 •
edited
Loading

Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new delete_unused_strings.py module, the strings_file_utils.py refactoring that extracts shared filter_entries logic, shell script integration, CLI subcommand additions, and comprehensive test suites. The detection strategy correctly scans for Localizations.X references (the SwiftGen-generated access pattern), normalizes keys to account for SwiftGen's identifier stripping, and handles multiline references. The conservative matching approach ensures false positives err on the side of retention rather than accidental deletion. Test coverage spans unit and integration levels, including edge cases for comment handling, case-insensitive matching, punctuation stripping, and file I/O behavior.

vvolkgang
vvolkgang reviewed Apr 9, 2026
View reviewed changes
vvolkgang
vvolkgang reviewed Apr 9, 2026
View reviewed changes
Scripts/fix-localizable-strings/delete_unused_strings.py


def _normalize_key(key: str) -> str:
"""Normalize a ``.strings`` key for comparison against a SwiftGen identifier.
Copy link
Copy Markdown
Member

@vvolkgang vvolkgang Apr 9, 2026

Choose a reason for hiding this comment

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

💭 Very personal take - given these scripts aren't for public distribution, these docstrings are blowing up line count without adding much, method name and the 3 comments above the regexps were enough for me at least.

Copy link
Copy Markdown
Contributor Author

@KatherineInCode KatherineInCode Apr 10, 2026

Choose a reason for hiding this comment

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

Our general MO in the codebase is to err on the side of documenting everything, even if it's private and small, so I'd prefer to keep with that trend. I can see the arguments either way, but in something like this, even if it's a pretty-niche script not for public distribution, it is still public, so I'd err on the side of over-documenting over under-documenting, especially since it calls out some of the caveats and logic and implications more than the regex alone does.

vvolkgang
vvolkgang requested changes Apr 9, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@vvolkgang vvolkgang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left some comments but nothing major here, we're just waiting for the Localizable.strings file to be updated following the merge of #2003

@github-actions github-actions bot removed app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Apr 10, 2026
@KatherineInCode KatherineInCode removed the t:ci Change Type - Updates to automated workflows label Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matt-livefront matt-livefront Awaiting requested review from matt-livefront matt-livefront is a code owner

@vvolkgang vvolkgang Awaiting requested review from vvolkgang

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

ai-review Request a Claude code review innovation Feature work related to Innovation Sprint or BEEEP projects t:feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KatherineInCode @vvolkgang