feat: Make empty string handling for optional params consistent#43
feat: Make empty string handling for optional params consistent#43FranciscoKurpiel merged 2 commits intomasterfrom
Conversation
This change introduces consistent behavior across the library for how optional parameters with default values handle empty strings (`""`) provided by configuration sources. Previously, the behavior was inconsistent: - `string` and `xtypes.String` would accept `""` as a valid value, overriding the default. - `xtypes.URL` would also override the default with an empty URL. - Most other basic types (`int`, `bool`, `time.Duration`, etc.) and xtypes (`xtypes.Bool`, `xtypes.Integer`) would fail with a parsing error. This commit aligns all non-string types to a single, predictable behavior. **Behavior Change:** For all optional parameters except `string` and `xtypes.String`, providing an empty string from a configuration source is now treated as an "absent" value. This causes the parameter to correctly fall back to its specified default value. - **Affected Types:** `time.Time`, `time.Duration`, `bool`, all numeric types (`int`, `uint`, etc.), `xtypes.URL`, `xtypes.Bool`, `xtypes.Integer`, and `xtypes.RawJSON`. - **Unaffected Types:** `string` and `xtypes.String` continue to treat `""` as a valid, intentional value. **Implementation Details:** - The `validFn` for the affected basic types and `ValueValid` for the affected xtypes were updated to return `types.ErrNoValue` when given an empty string. The parser uses this signal to ignore the value for optional parameters or error out for required ones. - The `TestEmptyURL` test was updated to reflect this new, stricter validation for required parameters. - New tests (`TestOptionalBasicTypes` and `TestOptionalXTypes`) were added to `parser_test.go` to provide comprehensive coverage for this behavior. - The `README.md` was updated to document this behavior for users.
Summary of ChangesHello @FranciscoKurpiel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant improvement in the library's configuration parsing by ensuring consistent and predictable behavior when optional parameters receive empty string values from configuration sources. The change aligns the handling of empty strings for most non-string types, allowing them to correctly utilize their default values instead of causing parsing errors or unintended overrides. This enhances the robustness and user-friendliness of the configuration system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable change by making the handling of empty strings for optional parameters consistent across the library. The approach of treating "" as an absent value for non-string types, causing them to fall back to their defaults, is a significant improvement in predictability and usability. The changes are well-implemented, and the addition of comprehensive tests in parser_test.go and updates to xtypes/url_test.go ensure the new behavior is well-covered. The documentation update in README.md is also clear and helpful for users. I have one suggestion regarding code duplication that could improve maintainability.
- Add a //nolint:gocyclo directive to suppress the cyclomatic complexity warning for the `configStandardCallbacks` function. - Run goimports on parser_test.go to fix formatting.
This change introduces consistent behavior across the library for how optional parameters with default values handle empty strings (
"") provided by configuration sources.Previously, the behavior was inconsistent:
stringandxtypes.Stringwould accept""as a valid value, overriding the default.xtypes.URLwould also override the default with an empty URL.int,bool,time.Duration, etc.) and xtypes (xtypes.Bool,xtypes.Integer) would fail with a parsing error.This commit aligns all non-string types to a single, predictable behavior.
Behavior Change:
For all optional parameters except
stringandxtypes.String, providing an empty string from a configuration source is now treated as an "absent" value. This causes the parameter to correctly fall back to its specified default value.time.Time,time.Duration,bool, all numeric types (int,uint, etc.),xtypes.URL,xtypes.Bool,xtypes.Integer, andxtypes.RawJSON.stringandxtypes.Stringcontinue to treat""as a valid, intentional value.Implementation Details:
validFnfor the affected basic types andValueValidfor the affected xtypes were updated to returntypes.ErrNoValuewhen given an empty string. The parser uses this signal to ignore the value for optional parameters or error out for required ones.TestEmptyURLtest was updated to reflect this new, stricter validation for required parameters.TestOptionalBasicTypesandTestOptionalXTypes) were added toparser_test.goto provide comprehensive coverage for this behavior.README.mdwas updated to document this behavior for users.