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

Add compatible command group #917

Merged
merged 14 commits into from
Oct 22, 2024
Merged

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Sep 26, 2024

Related: #926

Changelog

- description: |
    Introduce "compatible" command group
  type:
  - feature        # introduces a new feature
  - compatible     # the API has changed but is non-breaking

Context

Usage: cardano-cli compatible 
                                ( shelley
                                | allegra
                                | mary
                                | alonzo
                                | babbage
                                | conway
                                )

  Limited backward compatible commands for testing only.

Available options:
  -h,--help                Show this help text

Available commands:
  shelley                  Shelley era commands
  allegra                  Allegra era commands
  mary                     Mary era commands
  alonzo                   Alonzo era commands
  babbage                  Babbage era commands
  conway                   Conway era commands

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

@Jimbo4350 Jimbo4350 force-pushed the jordan/expose-compatible-command branch 3 times, most recently from c7f4302 to 903a192 Compare October 2, 2024 20:53
@Jimbo4350 Jimbo4350 force-pushed the jordan/expose-compatible-command branch from 903a192 to 8e4b6f4 Compare October 16, 2024 18:27
@Jimbo4350 Jimbo4350 changed the title Jordan/expose compatible command Add compatible command group Oct 16, 2024
@Jimbo4350 Jimbo4350 force-pushed the jordan/expose-compatible-command branch 6 times, most recently from 8bb861d to 65486f9 Compare October 17, 2024 14:39
@Jimbo4350 Jimbo4350 marked this pull request as ready for review October 17, 2024 14:39
@Jimbo4350 Jimbo4350 force-pushed the jordan/expose-compatible-command branch from 65486f9 to 5b37288 Compare October 17, 2024 15:11
CompatibleTransactionCmd cmd -> renderCompatibleTransactionCmd cmd
CompatibleGovernanceCmds cmd -> renderCompatibleGovernanceCmds cmd

pAnyCompatibleCommand :: EnvCli -> Parser AnyCompatibleCommand
Copy link
Contributor

@carbolymer carbolymer Oct 18, 2024

Choose a reason for hiding this comment

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

Parsers should go to cardano-cli/src/Cardano/CLI/Compatible/Options/Common.hs or something similar

Copy link
Contributor Author

@Jimbo4350 Jimbo4350 Oct 22, 2024

Choose a reason for hiding this comment

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

I don't want to mirror the previous EraBased directory structure. A Common module is fine but this parser is not common i.e it's called in one place.

Copy link
Contributor

@carbolymer carbolymer Oct 18, 2024

Choose a reason for hiding this comment

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

This file should be named
cardano-cli/src/Cardano/CLI/Compatible/Options/Governance.hs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, I don't want to mirror the previous directory structure.

Comment on lines +8 to +12
( CompatibleTransactionCmds (..)
, CompatibleTransactionError (..)
, pAllCompatibleTransactionCommands
, renderCompatibleTransactionCmd
, runCompatibleTransactionCmd
Copy link
Contributor

@carbolymer carbolymer Oct 18, 2024

Choose a reason for hiding this comment

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

This module should be split into Run, Option and Command modules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

What's the plan with Eons and compatible commands? Are we planning to keep necessary eons for those commands?

@Jimbo4350 Jimbo4350 force-pushed the jordan/expose-compatible-command branch from 5b37288 to 70c7079 Compare October 22, 2024 11:53
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Oct 22, 2024
Merged via the queue into main with commit 9197c62 Oct 22, 2024
25 checks passed
@Jimbo4350 Jimbo4350 deleted the jordan/expose-compatible-command branch October 22, 2024 13:09
@Jimbo4350
Copy link
Contributor Author

What's the plan with Eons and compatible commands? Are we planning to keep necessary eons for those commands?

Currently those eons will be intended for use only with our compatibility commands and we should add this to the haddocks in cardano-api. This will limit the scope of how much intra era compatibility code we have to write vs before where we had to write and maintain code for everything that changed between eras.

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