fix(config): support Chinese comma separator in allow_from environment variables#1301
Conversation
nikolasdehor
left a comment
There was a problem hiding this comment.
The intent is good -- supporting Chinese comma (,) as a separator in environment variables is a nice UX improvement for Chinese-speaking users who may accidentally use their IME's comma.
However, a few concerns:
-
No tests -- this is a parsing change that affects
allow_from, a security-sensitive configuration field. Unit tests forUnmarshalTextare essential. Test cases should cover:- English commas only:
"123,456,789" - Chinese commas only:
"123,456,789" - Mixed:
"123,456,789" - Trailing/leading commas:
",123,,456," - Single value (no commas)
- Empty string
- Whitespace:
" 123 , 456 "
- English commas only:
-
Scope creep risk -- should this also handle Japanese comma (
、), full-width comma (\uff0cwhich is the same as,), or semicolons? The current implementation is focused and reasonable, but consider documenting the supported separators. -
Interaction with JSON unmarshaling -- the existing
UnmarshalJSONhandles JSON arrays and numbers. The newUnmarshalTextis only used by theenvlibrary for environment variable parsing. This separation is correct, but please verify that theenvlibrary callsUnmarshalTextand notUnmarshalJSONwhen reading from environment variables. -
Make result nil vs empty slice consistent -- when text is empty, you return nil. When text is all commas (
",,,"), the result would be an empty[]string{}(allocated but empty). Consider whether nil vs empty-slice matters downstream.
Please add tests and this will be ready to approve.
|
@nicklein I've added comprehensive unit tests for the "FlexibleStringSlice.UnmarshalText" method as requested. Tests added in "pkg/config/config_test.go":
All tests have been verified to pass. The PR should now be ready for review and merge. Note: There may still be conflicts in docker-compose.yml that need to be resolved via rebase if the base branch has changed. |
f98d3ee to
18728b4
Compare
|
✅ Update: Conflicts resolved! The docker-compose.yml conflict has been resolved by rebasing onto the latest upstream/main. The PR now includes both:
The PR should now be mergeable. Please review when you have a chance. |
…t variables Add UnmarshalText method to FlexibleStringSlice to support both English (,) and Chinese (,) comma separators in environment variables. Includes comprehensive unit tests covering: - English commas, Chinese commas, mixed commas - Single values, whitespace trimming - Empty strings, edge cases Fixes sipeed#1280
18728b4 to
82756fa
Compare
|
✅ 分支已清理! 已移除与 PR #1301 无关的文件修改(Dockerfile.dev 和 docker-compose.yml 的更改)。 现在 PR 只包含以下修改:
PR 现在干净整洁,只关注解决中文逗号分隔符的问题。 |
yinwm
left a comment
There was a problem hiding this comment.
Review Summary
This PR correctly fixes a real user pain point for CJK users who accidentally use Chinese commas in environment variables. The implementation is clean and well-tested.
✅ What's Good
- Correctly implements
encoding.TextUnmarshalerinterface - Comprehensive test coverage (English/Chinese/mixed commas, whitespace, edge cases)
- Clear comments explaining the purpose
- Backward compatible
💡 Suggestions for Future Improvement
-
nil vs empty slice inconsistency: Empty string returns
nil, but comma-only input returns[]string{}. Consider unifying the behavior. -
JSON config support:
UnmarshalTextonly handles environment variables. If users configure via JSON file or Web UI with Chinese commas,UnmarshalJSONwon't handle it. Consider adding similar support there if needed. -
Integration test: Consider adding a test that verifies the env library correctly parses
PICOCLAW_CHANNELS_IRC_ALLOW_FROM="123,456".
These are minor suggestions and do not block merging. Thanks for the fix!
…t variables (sipeed#1301) Add UnmarshalText method to FlexibleStringSlice to support both English (,) and Chinese (,) comma separators in environment variables. Includes comprehensive unit tests covering: - English commas, Chinese commas, mixed commas - Single values, whitespace trimming - Empty strings, edge cases Fixes sipeed#1280
…t variables (sipeed#1301) Add UnmarshalText method to FlexibleStringSlice to support both English (,) and Chinese (,) comma separators in environment variables. Includes comprehensive unit tests covering: - English commas, Chinese commas, mixed commas - Single values, whitespace trimming - Empty strings, edge cases Fixes sipeed#1280
…t variables (sipeed#1301) Add UnmarshalText method to FlexibleStringSlice to support both English (,) and Chinese (,) comma separators in environment variables. Includes comprehensive unit tests covering: - English commas, Chinese commas, mixed commas - Single values, whitespace trimming - Empty strings, edge cases Fixes sipeed#1280
…t variables (sipeed#1301) Add UnmarshalText method to FlexibleStringSlice to support both English (,) and Chinese (,) comma separators in environment variables. Includes comprehensive unit tests covering: - English commas, Chinese commas, mixed commas - Single values, whitespace trimming - Empty strings, edge cases Fixes sipeed#1280
📝 Description
This PR fixes an issue where IRC (and other channels) allow_from configuration fails when users accidentally use Chinese comma (,) instead of English comma (,) as separators in environment variables.
The fix adds an UnmarshalText method to FlexibleStringSlice that:
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
Fixes #1280
📚 Technical Context
🧪 Test Environment
☑️ Checklist