Simplify by not using `QueryStakeAddressInfoCmdArgs` unnecessarily #1184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

newhoggy merged 3 commits into master from newhoggy/simplify-by-not-using-QueryStakeAddressInfoCmdArgs

May 16, 2025

Contributor

newhoggy commented May 14, 2025 •

edited

Loading

Changelog

- description: |
    Simplify code by not using `QueryStakeAddressInfoCmdArgs` unnecessarily.
    Also fixes `query spo-stake-distribution` command so that the output format flags are respected.
# 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

Don't build a value of a data type if it doesn't make sense to use all the fields. Just pass the values directly to the function instead.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

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


          Simplify by not using QueryStakeAddressInfoCmdArgs unnecessarily

45e8335

newhoggy requested review from Jimbo4350, carbolymer, palas, kevinhammond and disassembler as code owners

May 14, 2025 13:05

newhoggy commented

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Query/Run.hs

-                          { Cmd.commons = commons
-                          , addr
-                          , outputFormat
-                          , mOutFile -- unused anyway. TODO tighten this by removing the field.

Contributor Author

newhoggy May 14, 2025

Both outputFormat and mOutFile are unused by callQueryStakeAddressInfoCmd, so don't bother using QueryStakeAddressInfoCmdArgs and packing unnecessary fields.

newhoggy commented

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Query/Run.hs Outdated

                   spoToDelegatee <-
                     Map.fromList . concat
                       <$> traverse
                         ( \stakeAddr -> do
-                            info <- callQueryStakeAddressInfoCmd $ mkQueryStakeAddressInfoCmdArgs stakeAddr
+                            info <- callQueryStakeAddressInfoCmd commons stakeAddr

Contributor Author

newhoggy May 14, 2025

Instead call callQueryStakeAddressInfoCmd with the values it needs directly.

newhoggy commented

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Query/Run.hs Outdated

@@ @@ -1801,7 +1799,6 @@ runQuerySPOStakeDistribution @@
                       , Cmd.target
                       }
                   , Cmd.spoHashSources = spoHashSources'
-                  , Cmd.outputFormat

Contributor Author

newhoggy May 14, 2025

The unnecessary use of QueryStakeAddressInfoCmdArgs hides problems. For example here outputFormat is actually unused, when it really should be. The use of QueryStakeAddressInfoCmdArgs hides this fact.

newhoggy added 2 commits

May 14, 2025 23:09


          Rename callQueryStakeAddressInfoCmd to getQueryStakeAddressInfo becau…

0c56bcd

…se it is not a command function


          Make use of outputFormat field in the query spo-stake-distribution co…

01e646f

…mmand

newhoggy commented

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Query/Run.hs

                         [ ( spo
                           , coin
                           , Map.lookup spo spoToDelegatee
                           )
                         | (spo, coin) <- Map.assocs spoStakeDistribution
                         ]
-                  writeOutput mOutFile toWrite
+                      output =
+                        outputFormat

Contributor Author

newhoggy May 14, 2025

We now respect the output format of the query spo-stake-distribution command.

palas reviewed

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Query/Run.hs

-                  writeOutput mOutFile toWrite
+                      output =
+                        outputFormat
+                          & ( id

Contributor

palas May 14, 2025

Is this id to get alignment from the formatter? I don't think it has any effect on the code

Contributor Author

newhoggy May 15, 2025

Yes, it is purely for alignment purposes.

newhoggy requested a review from palas

May 15, 2025 09:34

Jimbo4350 approved these changes

View reviewed changes

newhoggy added this pull request to the merge queue

Merged via the queue into master with commit 9b851c5

26 checks passed

newhoggy deleted the newhoggy/simplify-by-not-using-QueryStakeAddressInfoCmdArgs branch

May 16, 2025 12:51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Jimbo4350 Jimbo4350 approved these changes

carbolymer Awaiting requested review from carbolymer carbolymer is a code owner

kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

disassembler Awaiting requested review from disassembler disassembler is a code owner

palas Awaiting requested review from palas palas is a code owner

Labels

None yet