Skip to content
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.

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

Clarify stake-pool queries with an "all or some"-logic #541

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

carlhammann
Copy link
Contributor

@carlhammann carlhammann commented Jan 3, 2024

Changelog

- description: |
    Make queries that optionally filter their result by stake pool keys more explicit:
    - `cardano-cli * query-stake-snapshot`
    - `cardano-cli * query pool-params` (which is superseded by the next one, but still present)
    - `cardano-cli * query pool-state`
    - `cardano-cli conway query stake-snapshot`
# 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
  - improvement    # QoL changes e.g. refactoring
  # - 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

This is a follow-up to #514, more concretely this comment. I try to make it obvious from the flags and help texts what the logic of the command is. As an example, consider

Usage: cardano-cli conway query stake-snapshot      ...

                                                 ( --all-stake-pools
                                                 | (--stake-pool-id STAKE_POOL_ID)
                                                 )
                                                 ...

  Obtain the three stake snapshots for a pool, plus the total active stake
  (advanced command)

Available options:
  ...

  --all-stake-pools        Query for all stake pools
  --stake-pool-id STAKE_POOL_ID
                           Stake pool ID/verification key hash (either
                           Bech32-encoded or hex-encoded).

  ...

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

@carlhammann carlhammann changed the title Clarify queries with an "all or some"-logic Clarify stake-pool queries with an "all or some"-logic Jan 3, 2024
@carlhammann carlhammann marked this pull request as ready for review January 3, 2024 14:40
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I completely understand why the Only constructor was converted to Some a [a] but I think the semantics you are looking for can be expressed with some: main...jordan/clarify-all-or-some-flags.

Specifically: ee94ab5

@carlhammann
Copy link
Contributor Author

carlhammann commented Jan 4, 2024

I'm not sure I completely understand why the Only constructor was converted to Some a [a] but I think the semantics you are looking for can be expressed with some

@Jimbo4350 Not sure why I renamed Only to Some either... it's probably an artifact of an earlier attempt. I'm aware of some, but optparse-applicative shows some, many, and optional in the same way, using [...]. I wanted to make really explicit that you'll have to provide at least one --stake-pool-id.

Edit: what I wrote above is untrue. optional and many are rendered the same, but that's not a problem here, since we only want to make visually obvious that some are needed. I'll follow your suggestion, then!

@carlhammann
Copy link
Contributor Author

@Jimbo4350 I changed to the simpler use of some in 04c23f6, as you proposed. Thanks for clearing my confusion there!

@@ -635,7 +635,7 @@ runQueryPoolStateCmd

let poolFilter = case allOrOnlyPoolIds of
All -> Nothing
Only poolIds -> Just $ Set.fromList poolIds
Some poolId poolIds -> Just $ Set.fromList (poolId : poolIds)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the same issue we spoke about previously. We can rely on some vs many here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you're looking at the most recent commit? I agreed with you on our previous discussion, and changed this is 4823900.

@carlhammann carlhammann added this pull request to the merge queue Jan 9, 2024
Merged via the queue into main with commit 6386a51 Jan 9, 2024
19 checks passed
@carlhammann carlhammann deleted the ch/clarify-all-or-some-flags branch January 9, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants