fcli icon indicating copy to clipboard operation
fcli copied to clipboard

FoD: Refactor argument options to lowercase

Open kadraman opened this issue 2 years ago • 5 comments

For consistency with other modules any FoD argument options should be changed to lowercase. For example:

fod app create-microservice-app --criticality=High --status=Development --owner=kevin.lee --microservices=ms1,ms2,ms3 --release-microservice=ms2 --auto-required-attrs fcli-test-a:1.0

should be changed to allow:

fod app create-microservice-app --criticality=high --status=development --owner=kevin.lee --microservices=ms1,ms2,ms3 --release-microservice=ms2 --auto-required-attrs fcli-test-a:1.0

I believe the API is case insensitive but all commands will need to be tested and test scripts updated.

kadraman avatar Jun 30 '23 12:06 kadraman

I agree that this would make the FoD module more consistent with other fcli modules. However, what about consistency with for example the -q option?

For example, based on your proposal, I guess we could end up with a situation like the following, where lowercase high needs to be used in the create command, but pascal-case High needs to be used in queries (using lowercase high in the query wouldn't return anything):

fod app create-microservice-app --criticality=high
fcli app list -q 'criticality=="High"'

rsenden avatar Jun 30 '23 13:06 rsenden

Yes, agreed but see below...

With your example, to get it to work in FoD you would actually need to do:

fcli fod app list -q "businessCriticalityType=='High'"

(powershell you need to swap single and double quotes)

even more confusing to query on the type, if you wanted to see web apps you would need to use the internal value:

fcli fod app list -q "applicationType=='Web_Thick_Client'"

As you know all the -q options can be found using -o json-properties but it does require knowledge of the FoD API to really understand what the values are.

Parking this one until we can or cannot come up with a better way.

kadraman avatar Jun 30 '23 14:06 kadraman

Potentially we could use record transformers to transform both property names and values, for example transform "applicationType": "Web_Thick_Client" to "type": "web". This would allow users to do fcli fod app list -q 'type=="web"'.

However:

  • This might be a lot of effort
  • We'll need to be able to do the reverse transformation as well, in case client-side queries get converted into server-side queries; we can already do this for property names but not property values
  • This may cause issues if FoD ever adds new properties that conflict with our renamed properties (for example, renaming applicationType to type would cause problems if FoD ever introduces a new type property)
  • This will cause a lot of confusion for people familiar with the FoD REST API

rsenden avatar Jun 30 '23 14:06 rsenden

As suggested in internal email, for ease of use and consistency with other modules, we'll probably want to use lower-case option values. Querying is a relatively advanced topic, especially given the comment from @kadraman above, so differences in case are probably the least of our worries.

In order to change current PascalCase option values to kebab-case values, enum values representing those option values should be changed to lowercase. For multi-word values, an _ will need to be inserted between each word, and ITypeConverter and default value provider will need to be configured on the appropriate options to convert _ to - and vice versa. See ProgressWriterTypeConverter and corresponding option configuration for the --progress option in ProgressWriterFactoryMixin. Ideally, at some point we should be able to handle such conversions in a generic way; see #306.

Obviously, this also depends on whether we have predefined enums listing allowed values for each option, or whether users would need to look up option values from FoD (see #361). If the latter, we'd either need to use the original FoD lookup values, or have the lookup command output both original PascalCase FoD lookup values (for use in queries, rest call command, ...), and those values converted to kebab-case (for use as option values).

rsenden avatar Jul 26 '23 15:07 rsenden

After closer inspection, this will likely require too much work to implement and maintain, and may reduce user experience due to inconsistencies with query/output expressions, so most likely we'll just stick to whatever format is used by the target system.

rsenden avatar Jul 31 '23 16:07 rsenden

Closing this issue, as we've long made the decision to stick to whatever format is used by the target system.

rsenden avatar May 29 '24 15:05 rsenden