create-poll, answer-poll, verify-poll: move to 'babbage governance' block #322

smelc · 2023-10-02T09:05:56Z

Changelog

- description: |
    create-poll, answer-poll, verify-poll: move to 'babbage governance' block
# 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

Part of fixing #313

Checklist

Commit sequence broadly makes sense and commits have useful messages
The change log section in the PR description has been filled in
New tests are added if needed and existing tests are updated. These may include:
- golden tests
- property tests
- round trip tests
- integration tests
  See Running tests for more details
NA The version bounds in .cabal files are updated
CI passes. See note on CI. The following CI checks are required:
- Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
- Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
- Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
Self-reviewed the diff

newhoggy · 2023-10-03T13:41:46Z

cardano-cli/test/cardano-cli-golden/Test/Golden/Babbage/Governance/AnswerPoll.hs

@@ -1,6 +1,6 @@
 {-# LANGUAGE OverloadedStrings #-}

-module Test.Golden.Shelley.Governance.AnswerPoll where
+module Test.Golden.Babbage.Governance.AnswerPoll where


The Shelley in Test.Golden.Shelley.Governance.AnswerPoll doesn't mean Shelley era, it means "Shelley Based Era", which is confusing.

When in era-based, we shouldn't organise by era because it can get complicated. We should omit both era and eon.

Maybe the module should beTest.Golden.EraBased.Governance.AnswerPoll to say we are testing era-based commands.

OK I see 👍

I changed to Test.Golden.EraBased.Governance.AnswerPoll.

cardano-cli/test/cardano-cli-golden/Test/Golden/Babbage/Governance/VerifyPoll.hs

newhoggy · 2023-10-03T13:55:35Z

cardano-cli/test/cardano-cli-golden/Test/Golden/Babbage/Governance/AnswerPoll.hs

-  expectedAnswerFile <- noteInputFile "test/cardano-cli-golden/files/golden/shelley/governance/polls/basic.answer.0.json"
+hprop_golden_babbageGovernanceAnswerPoll0 :: Property
+hprop_golden_babbageGovernanceAnswerPoll0 = propertyOnce . H.moduleWorkspace "governance-answer-poll" $ \tempDir -> do
+  pollFile <- noteInputFile "test/cardano-cli-golden/files/golden/babbage/governance/polls/basic.json"


This should be test/cardano-cli-golden/files/input/governance/polls/basic.json

That is:

Replace golden with input because this is not a golden file.

Delete babbage.

I did the change 👍

cardano-cli/test/cardano-cli-golden/Test/Golden/Babbage/Governance/AnswerPoll.hs

newhoggy · 2023-10-04T22:34:26Z

cardano-cli/src/Cardano/CLI/EraBased/Commands/Governance/Poll.hs

+      (Maybe Word) -- ^ Answer index
+      (Maybe (File () Out)) -- ^ Tx file
+  | GovernanceVerifyPoll  -- ^ Verify answer to a given SPO poll
+      (BabbageEraOnwards era) {- TODO smelc, use BabbageEraOnly here instead -}


Are these TODOs still relevant?

Yes they are, because the poll commands are babbage only, whereas this is babbage onwards.

cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Poll.hs

newhoggy · 2023-10-04T22:36:12Z

A few more comments. Merge when you're ready.

…lock

…ed once change lands in cardano-api.

newhoggy · 2023-10-05T10:28:43Z

cardano-cli/test/cardano-cli-golden/files/golden/help.cli

-                                                   [--out-file FILE]
-
-  Verify an answer to a given SPO poll
-


Actually, an oversight in my review. We're not actually supposed to delete the legacy commands. These need to be kept in tact. We will eventually delete all the legacy commands in one go, but not right now.

smelc force-pushed the smelc/clean-up-era-governance-commands-issue-313 branch 3 times, most recently from ad8be80 to f9f9f5f Compare October 2, 2023 14:06

smelc mentioned this pull request Oct 2, 2023

transaction sign complains when signing with cc-cold-keys #311

Closed

smelc force-pushed the smelc/clean-up-era-governance-commands-issue-313 branch 2 times, most recently from fc10f21 to ea30fe7 Compare October 2, 2023 19:04

smelc mentioned this pull request Oct 3, 2023

clean-up cardano-cli <era> governance commands #313

Closed

3 tasks

smelc force-pushed the smelc/clean-up-era-governance-commands-issue-313 branch 6 times, most recently from 886bdb5 to 3ac7b6f Compare October 3, 2023 11:42

smelc marked this pull request as ready for review October 3, 2023 12:31

smelc requested review from Jimbo4350, newhoggy, carbolymer, kevinhammond and disassembler as code owners October 3, 2023 12:31

smelc force-pushed the smelc/clean-up-era-governance-commands-issue-313 branch from 3ac7b6f to e5ed4f8 Compare October 3, 2023 12:31

smelc requested review from Jimbo4350, newhoggy, carbolymer, kevinhammond and disassembler as code owners October 3, 2023 12:31

newhoggy reviewed Oct 3, 2023

View reviewed changes

cardano-cli/test/cardano-cli-golden/Test/Golden/Babbage/Governance/VerifyPoll.hs Outdated Show resolved Hide resolved

newhoggy reviewed Oct 3, 2023

View reviewed changes

cardano-cli/test/cardano-cli-golden/Test/Golden/Babbage/Governance/AnswerPoll.hs Outdated Show resolved Hide resolved

newhoggy force-pushed the smelc/clean-up-era-governance-commands-issue-313 branch from e5ed4f8 to 55f92db Compare October 4, 2023 13:12

smelc force-pushed the smelc/clean-up-era-governance-commands-issue-313 branch 3 times, most recently from 8fabc26 to 5add478 Compare October 4, 2023 15:25

newhoggy reviewed Oct 4, 2023

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Poll.hs Outdated Show resolved Hide resolved

newhoggy approved these changes Oct 4, 2023

View reviewed changes

smelc added 9 commits October 5, 2023 09:57

create-poll, answer-poll, verify-poll: move to 'babbage governance' b…

59adca1

…lock

Update global help golden file

30729ea

Turn off 'cardano-cli conway governancy poll*' commands. To be revert…

56b1fca

…ed once change lands in cardano-api.

Update individual eras golden files

dc50e5f

Detail: adapt to cardano-api release

16cbf44

Adapt tests

b86b326

Remove babbage directory segment from a handful of locations

075c3aa

Better use of golden/input terms

20124f5

One missing renaming

834d66e

smelc force-pushed the smelc/clean-up-era-governance-commands-issue-313 branch from c847186 to 834d66e Compare October 5, 2023 07:58

smelc added this pull request to the merge queue Oct 5, 2023

Merged via the queue into main with commit 4c615c9 Oct 5, 2023
19 checks passed

smelc deleted the smelc/clean-up-era-governance-commands-issue-313 branch October 5, 2023 08:29

smelc mentioned this pull request Oct 5, 2023

Introduce BabbageEraOnly IntersectMBO/cardano-api#298

Closed

newhoggy reviewed Oct 5, 2023

View reviewed changes

This was referenced Oct 5, 2023

Plug *-poll legacy commands back #340

Closed

Plug *-poll legacy commands back #349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create-poll, answer-poll, verify-poll: move to 'babbage governance' block #322

create-poll, answer-poll, verify-poll: move to 'babbage governance' block #322

smelc commented Oct 2, 2023

newhoggy Oct 3, 2023 •

edited

Loading

smelc Oct 4, 2023

newhoggy Oct 3, 2023

smelc Oct 4, 2023

newhoggy Oct 4, 2023

smelc Oct 5, 2023

newhoggy commented Oct 4, 2023

newhoggy Oct 5, 2023

create-poll, answer-poll, verify-poll: move to 'babbage governance' block #322

create-poll, answer-poll, verify-poll: move to 'babbage governance' block #322

Conversation

smelc commented Oct 2, 2023

Changelog

Context

Checklist

newhoggy Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

smelc Oct 4, 2023

Choose a reason for hiding this comment

newhoggy Oct 3, 2023

Choose a reason for hiding this comment

smelc Oct 4, 2023

Choose a reason for hiding this comment

newhoggy Oct 4, 2023

Choose a reason for hiding this comment

smelc Oct 5, 2023

Choose a reason for hiding this comment

newhoggy commented Oct 4, 2023

newhoggy Oct 5, 2023

Choose a reason for hiding this comment

newhoggy Oct 3, 2023 •

edited

Loading