Teach pflag to handle unambiguous abbreviated long-args #372

opencoff · 2022-12-30T18:37:37Z

add a new abbreviation generator (abbrev.go) with tests
teach flag.go to build an abbrev table for long args and look it up and add tests to verify new behavior
Fix "spurious new line" error from use of Println()

* add a new abbreviation generator (abbrev.go) with tests * teach flag.go to build an abbrev table for long args and look it up and add tests to verify new behavior * Fix "spurious new line" error from use of Println()

CLAassistant · 2022-12-30T18:37:42Z

All committers have signed the CLA.

ccoVeille · 2025-06-24T20:47:57Z

example_test.go

@@ -7,7 +7,7 @@ package pflag_test
 import (
 	"fmt"

-	"github.com/spf13/pflag"
+	"github.com/opencoff/pflag"


This change seems to be a left behind debug

ccoVeille · 2025-06-24T20:48:30Z

flag.go

@@ -16,7 +16,7 @@ pflag is a drop-in replacement of Go's native flag package. If you import
 pflag under the name "flag" then all code should continue to function
 with no changes.

-	import flag "github.com/spf13/pflag"
+	import flag "github.com/opencoff/pflag"


ccoVeille · 2025-06-24T20:49:13Z

flag_test.go

@@ -1314,7 +1314,7 @@ func TestPrintDefaults(t *testing.T) {
 func TestVisitAllFlagOrder(t *testing.T) {
 	fs := NewFlagSet("TestVisitAllFlagOrder", ContinueOnError)
 	fs.SortFlags = false
-	// https://github.com/spf13/pflag/issues/120
+	// https://github.com/opencoff/pflag/issues/120


ccoVeille · 2025-06-24T20:49:29Z

go.mod

@@ -1,3 +1,3 @@
-module github.com/spf13/pflag
+module github.com/opencoff/pflag


ccoVeille · 2025-06-24T20:50:13Z

abbrev_test.go

+}
+
+// make an assert() function for use in environment 't' and return it
+func newAsserter(t *testing.T) func(cond bool, msg string, args ...interface{}) {


t.Helper is missing here

Suggested change

func newAsserter(t *testing.T) func(cond bool, msg string, args ...interface{}) {

func newAsserter(t *testing.T) func(cond bool, msg string, args ...interface{}) {

t.Helper()

tomasaschan

The implementation looks (mostly, but not entirely) ok, given that line comments are addressed appropriately.

I'm not convinced the feature is a good idea, though.

My main reason to hesitate is that this is makes it a lot easier for users of pflag to, unintendedly, introduce changes in their CLI contracts that are breaking to their users. Most obviously, adding a new flag that shares a prefix with an existing one, will break all scripts (or shell history) that use an abbreviation which is no longer unambiguous.

One could argue that it's not pflag's responsibility to ensure our users don't break their users, but having this feature as is - enabled and non-optional - effectively makes it harder for our users not to. I'm not convinced of the value of this feature either way, but I think it needs to, at the very least, be behind a feature flag and turned off by default.

I don't know if pflag already has a mechanism for users to toggle features on or off. If there is no such mechanism, building one is probably a good scope for a separate PR independent of this one.

tomasaschan · 2025-06-25T07:22:01Z

abbrev.go

+// abbrev.go - generate abbreviations from a wordlist
+//
+// (c) 2014 Sudhi Herle <sw-at-herle.net>
+//
+// Placed in the Public Domain
+// This software does not come with any express or implied
+// warranty; it is provided "as is". No claim  is made to its
+// suitability for any purpose.


Is this code copied from somewhere else? If so where?

Might just be me, but I also find the combination of a copyright header and "added to the public domain" confusing. Is it copyright protected or not? What are the expectations on anyone finding this piece of code and wanting to use it?

The original inspiration came from perl's abbrev.pm; IIRC I picked the same terms as that module.

tomasaschan · 2025-06-25T07:22:52Z

abbrev.go

+//	Abbrev() returns:
+//	  {
+//	     "hello": "hello",
+//	     "hell":  "hell"


This should read "hell": "hello",?

tomasaschan · 2025-06-25T07:24:14Z

abbrev.go

+			case 2:
+				delete(table, ab)
+			default:
+				goto next


Hm. No fan of this. Why not use a (possibly labeled) break?

tomasaschan · 2025-06-25T07:28:23Z

go.mod


-go 1.12
+go 1.24.3


This is, if intended, a (significantly) breaking change. Is it necessary?

opencoff · 2025-06-25T17:26:18Z

The implementation looks (mostly, but not entirely) ok, given that line comments are addressed appropriately.

I'm not convinced the feature is a good idea, though.

My main reason to hesitate is that this is makes it a lot easier for users of pflag to, unintendedly, introduce changes in their CLI contracts that are breaking to their users. Most obviously, adding a new flag that shares a prefix with an existing one, will break all scripts (or shell history) that use an abbreviation which is no longer unambiguous.

One could argue that it's not pflag's responsibility to ensure our users don't break their users, but having this feature as is - enabled and non-optional - effectively makes it harder for our users not to. I'm not convinced of the value of this feature either way, but I think it needs to, at the very least, be behind a feature flag and turned off by default.

I don't know if pflag already has a mechanism for users to toggle features on or off. If there is no such mechanism, building one is probably a good scope for a separate PR independent of this one.

Your points re defaulting to "on" leading to breakages is valid. Perhaps there is no point in pursuing this PR.

Teach pflag to handle abbreviated long-args:

66c96e5

* add a new abbreviation generator (abbrev.go) with tests * teach flag.go to build an abbrev table for long args and look it up and add tests to verify new behavior * Fix "spurious new line" error from use of Println()

opencoff added 4 commits March 8, 2023 11:36

Update mod name for local fork; move go version forward

2d3c2ad

Fix missed references to spf13 in packages and docs

96e19b4

Removed stale go.sum

ffff501

Merge remote-tracking branch 'upstream/master'

a2eda92

ccoVeille reviewed Jun 24, 2025

View reviewed changes

tomasaschan reviewed Jun 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Teach pflag to handle unambiguous abbreviated long-args #372

Teach pflag to handle unambiguous abbreviated long-args #372

Uh oh!

opencoff commented Dec 30, 2022

Uh oh!

CLAassistant commented Dec 30, 2022 •

edited

Loading

Uh oh!

ccoVeille Jun 24, 2025

Uh oh!

ccoVeille Jun 24, 2025

Uh oh!

ccoVeille Jun 24, 2025

Uh oh!

ccoVeille Jun 24, 2025

Uh oh!

ccoVeille Jun 24, 2025

Uh oh!

tomasaschan left a comment

Uh oh!

tomasaschan Jun 25, 2025

Uh oh!

opencoff Jun 25, 2025

Uh oh!

tomasaschan Jun 25, 2025

Uh oh!

tomasaschan Jun 25, 2025

Uh oh!

tomasaschan Jun 25, 2025

Uh oh!

opencoff commented Jun 25, 2025

Uh oh!

Uh oh!

		@@ -1,3 +1,3 @@
		module github.com/spf13/pflag
		module github.com/opencoff/pflag

Teach pflag to handle unambiguous abbreviated long-args #372

Are you sure you want to change the base?

Teach pflag to handle unambiguous abbreviated long-args #372

Uh oh!

Conversation

opencoff commented Dec 30, 2022

Uh oh!

CLAassistant commented Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomasaschan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opencoff commented Jun 25, 2025

Uh oh!

Uh oh!

CLAassistant commented Dec 30, 2022 •

edited

Loading