Repository navigation
Preserve current string values in CopyToGoFlagSet - #520
adenzhou1350 wants to merge 2 commits into
Conversation
Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
|
aden seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
@tomasaschan Could you approve the CI workflow when convenient? This is a one-line follow-up to #330; the string branch now preserves the current value like the other scalar branches. Thanks! |
|
@adenzhou1350 I was waiting for the CLA signature. This project gets enough automated slop these days that I don't really look at PRs until the CLA is signed - it's so far been a fairly trustworthy signal for whether there's actually someone who'll eg respond to feedback at the other end. |
|
Thanks for clarifying. I have flagged the pending CLA signature for the account holder, and understand that review is waiting on it. Separately, all six test jobs pass. The Lint job fails while importing Go 1.27.2 standard-library export data: golangci-lint 2.13.2 reports export data version 5 greater than its maximum supported version 4. This looks like a CI toolchain compatibility issue rather than evidence of a failure in the string-copy change: https://github.com/spf13/pflag/actions/runs/38585875801/job/115938404479?pr=520 |
|
@tomasaschan CLAassistant now confirms that all committers have signed, and license/cla is green. The six Go test jobs are also green; the remaining lint/toolchain failure is detailed above. Could you take a look at the string-copy fix when convenient? Thanks. |
|
Looks like the linter might need a version bump, similar to #508 |
Signed-off-by: aden <aden@example.com>
|
Thanks, I followed the same approach in 7fbce12 and bumped the workflow from v2.13 to v2.14. On Windows with Go 1.27.2, the official 2.13.2 binary reproduces the export-data v5/v4 failure, while 2.14.0 passes with 0 issues on the same LF source snapshot. Config verification and go test ./... also pass. The change is limited to the linter version line; hosted CI still needs to confirm the new head. |
CopyToGoFlagSet registers string flags using their original DefValue. Because flag.StringVar initializes the shared variable, copying a parsed string (including an explicit empty string) silently resets it. Use the current value, matching the other scalar branches. Related #330.
The regression test covers default, parsed and empty values, preserves the source default, and checks shared updates in both directions.
Validation: Linux full race suite passes on Go 1.26.9 (205 test/subtest results); native Windows focused cases pass. The parsed/empty regressions fail with unchanged production code on both platforms. Full Windows/race and the upstream Go-version/lint matrix were not run. An existing TestGoflags flag-redefinition failure when repeating the suite in one process occurs in both arms; fresh-process suites pass.
AI-assisted investigation and tests; the one-line production change and regression coverage were independently reviewed.