snivilised / cobrass Goto Github PK

See: coveralls/go

eg, currently we have BindValidatedContainsEnum/BindValidatedEnumNotContains, but is that the correct naming?

Change the generator so we can add custom documentation for a binder. Currently this is required for BindEnum and BindValidatedEnum (see the extra comments at the end of param-set.go)

• gen-ov: generates types and funcs
• gen-ps: generates funcs
• gen-help: generates funcs

Make sure you don't publish enum methods that don't make sense without explicit new functionality. Some methods may need to be deactived in the generator script until custom functionality has been designed. Example of a method that should not be published as it are the comparison functions. This is because the enum binders work with the string values and for each enum value there can be multiple acceptable values, complicating the matter further still. Any comparison is performed in the string domain not the underlying numeric domain that the user might be expecting. This means the result of any string domain comparison may not be consistent when you compare the underlying int values. So for now, just mnake sure we don't accidently publish generated api functions that don't make sense.

VaildatorGroup is a bad name and a bit of a misnomer. Should change to VaildatorCollection or perhaps Vaildators or ValidatorContainer

duration slice needs to be enabled and full api and tests for duration needs to be generated.

cmd.PersistentFlags is similar to cmd.Flags, but defines flags that can be inherited.

Change the generator (Build-ParamSet) to generate altered code, that will conditionally invoke either FlagSet or PersistentFlags depending on whether the flaginfo.IsPersistent bool is set (clearly need to add IsPersistent to flag info).

The client can still use the same ParamSet as for non persistent flags.

See:

• using-forked-package-import-in-go
• forking-golang-repositories-on-github-and-managing-the-import-path

show-sig, simply shows the signature. We can make this more useful, if we added a hash compare facility to it via an environment variable, let's say COBRASS_HASH.

This variable can be set in the profile,so it doesn't have to keep being set manually. The only time this has to change, is when there is a legitimate change to the auto generated api.

show-sig, will calculate the current has value and compare with that defined in the variable, if it has been set and report an error if they don't match.

Currently, print the value of an enum just prints the interer number, which is not helpful. Using reflection, we should be able to get the enum name

There was more work than was originally envisioned for the original issue #25, which was only partly completed and pushed so as to not have too much outstanding functionality un-pushed.

Still to be completed in the powershell generator script:

• documentation comments in Build-Predefined/gen-help
• BhTests for all type insances

requires more test cases to be generated, not all bases have been covered

Replaces NewParameterSet.

The parameter set should be seen differently now. Think in terms of what a parameter set is in powershell, where each command can multiple mode of operation, where each mode is a parameter set.

When we think this way, each cobra sub command can have multiple modes of operation. The client can either use a single Parameter set definition for all modes of operation for a sub command, eg InfexCommandSet or have muliple different parameter sets InfexCommandAlphaSet, `InfexCommandBetaSet' etc.

The point is, whatever is passed in as a set of generic parameters, doesn't necessarily have to have an entry for all members of the native object, so instead of panicing, we should move on without setting that value. This would leave some of the native members to be their zero value.

It therefore stands to reason that a client should really define muliple parameter sets for each sub command. Also, if the cli defines default values for some of the members, then these defaults must be set, probably in the generic parameter set.

To ensure consistency with cross field validation, there should be a validate method on the param-set, to ensure validation is consistent

have to make sure that once the package is published, any changes to the generation script do not accidentally change function/variable/type definitions or their names as this will break clients.

• #37

There are currently no validators for enum types. This is because they of the discrepancy of comparability between int and string. However, some operators are still valid and therefore should be implmeneted:

• BindValidatedEnumContains/NotContains
• BindValidatedEnumIsValid - CUSTOM, validates against teh enuminfo (might not be possible, if we can't define a generic type on the method)

on cobra container, some methods panic, and return error. This needs to be reviewed and make sure that the api is consistent

And correct some spelling mistakes

It clearly does not make sense for a bool option to be validated as it can only be true or false.

• BindValidatedBool
• BindValidatedBoolSlice

Also, some some types are missing Slice definition because they are missing the GenerateSlice flag, so correct them too.

This aleviates the client from having to manage another package global variable

change the dump to return a string instead of writing to stdout. This then gives the user the choice of what to do with the dump content.

There will need to be 1 for each type, String, Bool, Int, Int32 etc. These will have to be wrapped inside a collection. Then the binder functions on the param-set, should be changed to accept an extra parameter, that being the individual option validator.

If you are using the param-set, then as each flag is bound, the validator is added to the collection. Then the registered Arg validator function should invoke the validate all on the collection, failing on the first error encountered.

By what if the client is not using the param set? Well, we could let the client use the collection externally. The advantage of using the param set is that the client would not need to have to work with the validator collection as that is handled for them.

We probably need to flesh out the param set to also include extra methods to invoke the non short form versions.

The client needs to create the generic parameter map using reflection to ensure that the type of the value inserted matches the corresponding type of the native member. This is required because VisitAll always presents values as strings and this will fail when the corresponding native member is not a string.

The current panic looks like this:

--- FAIL: TestInfexCommand (0.00s)
panic: reflect.Set: value of type *pflag.stringValue is not assignable to type string [recovered]
	panic: reflect.Set: value of type *pflag.stringValue is not assignable to type string

goroutine 6 [running]:
testing.tRunner.func1.2({0x772200, 0xc00005f930})
	/usr/local/go/src/testing/testing.go:1389 +0x24e
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1392 +0x39f
panic({0x772200, 0xc00005f930})
	/usr/local/go/src/runtime/panic.go:838 +0x207
reflect.Value.assignTo({0x7926a0?, 0xc00005f240?, 0x0?}, {0x7e2808, 0xb}, 0x772200, 0x0)
	/usr/local/go/src/reflect/value.go:3064 +0x2ac
reflect.Value.Set({0x772200?, 0xc000072970?, 0x74b02a?}, {0x7926a0?, 0xc00005f240?, 0x772200?})
	/usr/local/go/src/reflect/value.go:2090 +0xeb
github.com/snivilised/cobrass/src/adapters.NewParameterSet[...](0xc000010018)
	/home/plastikfan/go/pkg/mod/github.com/snivilised/[email protected]/src/adapters/parameter-set.go:37 +0x1b5
github.com/snivilised/squif/src/infex.NewInfexParameterSet(...)
	/home/plastikfan/dev/github/go/snivilised/squif/src/infex/infex-parameter-set.go:20
github.com/snivilised/squif/src/infex.RunInfex(0xc000190280?, {0xc000068720, 0x1, 0x2?})
	/home/plastikfan/dev/github/go/snivilised/squif/src/infex/run-infex.go:44 +0x1a5
github.com/spf13/cobra.(*Command).execute(0xc000190280, {0xc000068700, 0x2, 0x2})
	/home/plastikfan/go/pkg/mod/github.com/spf13/[email protected]/command.go:856 +0x67c
github.com/spf13/cobra.(*Command).ExecuteC(0xc000190000)
	/home/plastikfan/go/pkg/mod/github.com/spf13/[email protected]/command.go:974 +0x3b4
github.com/snivilised/cobrass/src/testhelpers.ExecuteCommandC(0xc000190000, {0xc00007fce0, 0x3, 0x3})
	/home/plastikfan/go/pkg/mod/github.com/snivilised/[email protected]/src/testhelpers/cobra-test-helpers.go:39 +0xde
github.com/snivilised/cobrass/src/testhelpers.ExecuteCommand(...)
	/home/plastikfan/go/pkg/mod/github.com/snivilised/[email protected]/src/testhelpers/cobra-test-helpers.go:16
github.com/snivilised/squif/src/app/cobracmd.TestInfexCommand(0x0?)
	/home/plastikfan/dev/github/go/snivilised/squif/src/app/cobracmd/infex_test.go:19 +0xf8
testing.tRunner(0xc000113520, 0x80c7f0)
	/usr/local/go/src/testing/testing.go:1439 +0x102
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1486 +0x35f
FAIL	github.com/snivilised/squif/src/app/cobracmd	0.006s
FAIL

with no indication of which native member could not be set because of the mismatching type.

Actually, as can been seen, VisitAll returns a pointer to a string for strings, so strings dont even work!

The panic message should indicate which native member could not be set.

This can't be done by defining an interface in cobrass, because that would defeat the whole point of this functionality. The client wouldl then have a dependency on cobrass which is no better than having a dependency on Cobra. It would also probably lead to interface pollution which we must avoid.

The way to do this is to define a method on the cobra collection that takes a callback as one of the parameters. The client defined callback would be notified of the commands and would be able to create a client side abstraction populated from information found in the commands. The result of this operation would be another client side collection that can safely injected into the client app,thereby breaking coupling to Cobra.

The trickiest part is the fact that Cobra commands are hierarchical as opposed to being flat as they are in Cobrass. So we need to walk a tree, rather than iterate a linear sequence. So Cobrass, will walk the tree and invoke the call back with the parent and the current child.

Remember, the onlypoint of this is to be able to access the cli flags, not the behavioural aspects of the cobra commands, so perhaps, we don't need to maintain the tre hierarchy.

Also, the trasversal should be on a per command basis. So if the client want information fo rmultiple commands, then they will have to perform multiple ejections, one for each command and actually,perhaps this is how we solve our hierarchy problem. We just flatten the structure out based upon each individual command.

I'm not sure that I like writing code that looks like this:

	infexCommand.Flags().StringVarP(
		&FormatFlag,
		"format",
		"f",
		defaultFormat,
		fmt.Sprintf(
			"infexion format [xml|json|text|scribble] (optional, def=%v)",
			defaultFormat),
	)

The pflags api looks a bit esoteric and the code wriiten doesn't look particularly clean. This is probably due to the fact that it was written prior to the availability of generics in go, giving rise to methods like StringVarP which defines a string flag, with a shortcut assignable to an external variable.

Assuming the client never wants to use the var version of the methods, since we can get hold of values by visiting the flag set, we can define simpler wrapper methods with more expressive names.

Eg instead of StringVarP, let's define ShortString/String methods on the Cobra container. Ideally, we'd use generics here, but we;re restricted by the fact that the underlying pflags library is not a generic api.

Having taken a look more deeply into the pflag api, its going to require a lot of code to wrap all the available functions, so might not implement this as the payback is too low for the effort required.

This does not have to be implemented straight away. Its enough just to do some analysis and a design to make sure that it can be implemented in the future, prior to version 1.0 being released.

The design should not encroach on validation group checks that have recently released in cobra, so its not about flag mutual exclusion or flags being mandatory depening on the presence of another. These types of checks are about if X='n', then Y can only be x, y or z, or if X defines a value, then Y cannot be less than X.

So the validation helpers that have already been defined are based on static limits, where as the cross validation checks are based on dynamic values defined by other flags.

When the client uses VisitAll method (command.Flags().VisitAll), the flag names appear with the initial characher non capitalised, which does not match the names of the properities on the native object. The fields of the native object are reflected based upon the names of the native object field name (used to key into a map) which mismatches because of initial character non captilisation.

NewParameterSet should panic with a useful error message indicating that the lookup into the generic map failed.

The current panic looks like this:

--- FAIL: TestParameterSetSuite (0.00s)
    --- FAIL: TestParameterSetSuite/TestNativeObjectParamMismatchWithGenericEntry (0.00s)
        /home/plastikfan/dev/github/go/snivilised/cobrass/src/adapters/suite.go:63: test panicked: reflect: call of reflect.Value.Set on zero Value
            goroutine 7 [running]:
            runtime/debug.Stack()
            	/usr/local/go/src/runtime/debug/stack.go:24 +0x65
            github.com/stretchr/testify/suite.failOnPanic(0xc000115ba0)
            	/home/plastikfan/go/pkg/mod/github.com/stretchr/[email protected]/suite/suite.go:63 +0x3e
            panic({0x7b1420, 0xc00000ea68})
            	/usr/local/go/src/runtime/panic.go:838 +0x207
            reflect.flag.mustBeExportedSlow(0x77c94d?)
            	/usr/local/go/src/reflect/value.go:237 +0xc5
            reflect.flag.mustBeExported(...)
            	/usr/local/go/src/reflect/value.go:231
            reflect.Value.Set({0x7a07e0?, 0xc000073500?, 0x77c94d?}, {0x0?, 0x0?, 0x7a07e0?})
            	/usr/local/go/src/reflect/value.go:2085 +0x9f
            github.com/snivilised/cobrass/src/adapters.NewParameterSet[...](0xc000139578)
            	/home/plastikfan/dev/github/go/snivilised/cobrass/src/adapters/parameter-set.go:29 +0x1b5
            github.com/snivilised/cobrass/src/adapters.NewFooParameterSet(...)
            	/home/plastikfan/dev/github/go/snivilised/cobrass/src/adapters/parameter-set_test.go:89
            github.com/snivilised/cobrass/src/adapters.(*ParameterSetSuite).TestNativeObjectParamMismatchWithGenericEntry(0x0?)
            	/home/plastikfan/dev/github/go/snivilised/cobrass/src/adapters/parameter-set_test.go:152 +0x2f7
            reflect.Value.call({0xc000073140?, 0xc000010ce0?, 0x13?}, {0x810f3a, 0x4}, {0xc00005be70, 0x1, 0x1?})
            	/usr/local/go/src/reflect/value.go:556 +0x845
            reflect.Value.Call({0xc000073140?, 0xc000010ce0?, 0xc000166080?}, {0xc00004a670, 0x1, 0x1})
            	/usr/local/go/src/reflect/value.go:339 +0xbf
            github.com/stretchr/testify/suite.Run.func1(0xc000115ba0)
            	/home/plastikfan/go/pkg/mod/github.com/stretchr/[email protected]/suite/suite.go:158 +0x4b6
            testing.tRunner(0xc000115ba0, 0xc0001c0000)
            	/usr/local/go/src/testing/testing.go:1439 +0x102
            created by testing.(*T).Run
            	/usr/local/go/src/testing/testing.go:1486 +0x35f
FAIL
exit status 1
FAIL	github.com/snivilised/cobrass/src/adapters	0.003s

which gives no indication this is was because of the name mismatch. Instead we see

call of reflect.Value.Set on zero Value

which is presumably because this line:

value := params[name]

yields a zero value.

Currently, there is a value associated with the enum info. I just relised this is incorrect as it basically means there can only ever be 1 value that type in a program which is clearly not desirable.

The enum info should be seen as the backing info for associated enum values. We need another instance, let's say enum value which contains the Source member which is bound to using the binder functions, but also has a pointer to the enum info.

This is an additional helper for the client that can be used instead of CreateParameterSet. CreateParameterSet requires the user to manually collate generic parameters using the appropriate Visit function on cobra's FlagSet. However, this should not be necessary, although it can be used for extra control.

ExtractParameterSet will perform the appropriate extraction in one go without the client needing to anything other than invoke this function passing in the cobra command's FlagSet.

To be used by the client cli using the cobra visitall facilty. The client creates a generic map of parameter name to value, then creates a native parameter set object with all the fields in the map populated on the native representation. Requires reflection

See: ginkgo

See:

• How to write github actions for your golang projects - github actions tutorial

• github-actions-for-go-projects

• awesome-actions

• act