25 general dynamically load language list#42
Conversation
…anges This reverts commit da32d09 - "Replace hard-coded language validation with dynamic ResourceManager-based lookup".
There was a problem hiding this comment.
Pull request overview
This PR updates PasswordValidatorService to determine supported language codes dynamically from the project’s compiled resource sets (instead of a hardcoded switch), and adds unit tests to validate language-code acceptance/rejection behavior.
Changes:
- Replace hardcoded supported-language switch with a lazy, resource-backed supported language set.
- Add unit tests covering supported, unsupported, and culture-specific (e.g.,
en-US) language codes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Easy-Password-Validator/PasswordValidatorService.cs | Builds supported languages from available Resources.*.resx via ResourceManager, and uses it to validate languageCode. |
| Easy-Password-Validator-Test/PasswordValidatorTests.cs | Adds test coverage for supported/unsupported language codes and common culture-code formats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try | ||
| { | ||
| return _rm.GetResourceSet(c, true, false) != null; | ||
| } | ||
| catch | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This is a good suggestion. I'm not sure how specific we want to be with exception handling here. @thirstyape please provide guidance.
There was a problem hiding this comment.
What other exceptions can occur? It's probably not worth implementing this feature at all if we are introducing the potential for new exceptions.
There was a problem hiding this comment.
Based on my research, GetResourceSet(System.Globalization.CultureInfo culture, bool createIfNotExists, bool tryParents) may throw mainly three exceptions:
ArgumentNullException: only ifculture== null, won't happenMissingManifestResourceException: resources not found with fallback rulesMissingSatelliteAssemblyException, a culture-specific resource is expected but missing, less common
These should be rare in a correctly built package?
Since this is just a feature check (whether a language is supported), it seems reasonable to treat any failure as “not supported” and use a general catch that returns false, rather than introducing new exceptions for consumers, i.e. maybe we can ignore this suggestion?
There was a problem hiding this comment.
If we don’t want to introduce this extra complexity, I think we can keep the original explicit contract and just express it more cleanly with a static HashSet<string> instead of a switch.
| else if (languageCode.Length == 5 && languageCode[2] == '-') | ||
| { | ||
| return _supportedLanguages.Value.Contains(languageCode.Substring(0, 2)); | ||
| } |
There was a problem hiding this comment.
Not sure if I should touch the original way of checking. @thirstyape please suggest.
There was a problem hiding this comment.
Copilot has it backwards based on the library design. We only add region-specific when there is already a 2-letter version. Not entirely clear based on the default being en. In the resource files, Resources.resx is en, then there is also en-CA and en-US for things like neighbor vs neighbour.
In general, if we were to accept a PR for fr-CA we would leave the existing fr, or if someone wanted to submit something such as pt-BR we would change the name to pt as there is not already anything existing for that.
In short, you can ignore Copilot, either what you have here or what was originally there are good.
No description provided.