transaction id: add --output-[json,text] flag to control format of the output

[smelc]

Changelog

- description: |
    transaction id: add --output-[json,text] flag to control format of the output
# uncomment types applicable to the change:
  type:
  - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

• Part of fixing [FR] - Make output of CLI commands consistent. #566
• Fixes era transaction txid: add --output-json and --output-text #1001
• Way to get me back to coding after the Christmas break 😊

How to trust this PR

• New tests are added
• You can try the new test before the commit introducing the feature, and see it still works on HEAD so the default behavior didn't change

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• New tests are added if needed and existing tests are updated. See Running tests for more details
• Self-reviewed the diff

[palas]

Looks good. I just have two suggestions.

[palas]

I think it would be best to have the default be a separate variable and compare to it, since we cannot ensure that there are no several True values in this list. Also the parser could already return the default instead of a Maybe, cause that would avoid having coupling with the implementation code.

[smelc]

I think it would be best to have the default be a separate variable and compare to it, since we cannot ensure that there are no several True values in this list

@palas> done:

@@ -1728,14 +1728,15 @@ pOutputFormatJsonOrText kind =
 pTxIdOutputFormatJsonOrText :: Parser OutputFormatJsonOrText
 pTxIdOutputFormatJsonOrText =
   asum
-    [ make OutputFormatJson "JSON" "json" False
-    , make OutputFormatText "TEXT" "text" True
+    [ make OutputFormatJson "JSON" "json"
+    , make OutputFormatText "TEXT" "text"
     ]
  where
-  make format desc flag_ default_ =
+  default_ = OutputFormatText
+  make format desc flag_ =
     Opt.flag' format $
       mconcat
-        [ Opt.help $ "Format output as " <> desc <> (if default_ then " (the default)." else ".")
+        [ Opt.help $ "Format output as " <> desc <> (if format == default_ then " (the default)." else ".")
         , Opt.long ("output-" <> flag_)
         ]
 
diff --git a/cardano-cli/src/Cardano/CLI/Types/Common.hs b/cardano-cli/src/Cardano/CLI/Types/Common.hs
index 87253b0c7..d23ee97f2 100644
--- a/cardano-cli/src/Cardano/CLI/Types/Common.hs
+++ b/cardano-cli/src/Cardano/CLI/Types/Common.hs
@@ -545,7 +545,7 @@ data TxMempoolQuery
 data OutputFormatJsonOrText
   = OutputFormatJson
   | OutputFormatText
-  deriving Show
+  deriving (Eq, Show)

Also the parser could already return the default instead of a Maybe, cause that would avoid having coupling with the implementation code.

@palas> could we? Because we want the flags to be optional, to preserve the existing behavior.

[palas]

Yes, you can have a parser that returns the default on absence of a flag. I have done it somewhere, let me check

[smelc]

@palas> ah yes I could simply lift a value to Parser at the level of a Parser (Maybe a). I have the same issue for #1010 🙂

[smelc]

@palas> this works (for #1010):

pDRepIdOutputFormat :: Parser IdOutputFormat
pDRepIdOutputFormat =
  asum [make IdOutputFormatHex "hex", make IdOutputFormatBech32 "bech32"]
    <|> pure default_
 where
  default_ = IdOutputFormatBech32
  make format flag_ =
    Opt.flag' format $
      mconcat
        [ Opt.help $
            "Format drep id output as "
              <> flag_
              <> (if format == default_ then " (the default)." else ".")
        , Opt.long ("output-" <> flag_)
        ]

[palas]

That is it, I had forgotten the exact details

[Jimbo4350]

LGTM. Was this requested?

[smelc]

LGTM. Was this requested?

@Jimbo4350> no, I witnessed this was a possible improvement to consistency of our CLI interface when using the id command for another work.

[palas]

Thanks 👍