Structuring legacy code so that era-based code does not depend on it

[newhoggy]

Please read the "Context" and "Reading this PR" sections before reviewing the code.

Changelog

- description: |
    Structuring legacy code so that era-based code does not depend on it
# 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

Currently, we have some modules that are "legacy" and serve to implement the legacy command structure and other modules that are "era-based" modules that serve to implement the era-based command structure.

Occasionally these modules were located in the wrong place (for example some legacy modules were found to be under EraBased. Furthermore there is no "home" for the legacy module.

This PR aims to do four main things:

• Re-organise the code so that all the era-based code is under Cardano.CLI.EraBased and all the legacy code is under Cardano.CLI.Legacy.
• Change the dependency between modules so that era-based modules never depend on legacy modules.
• Allow sharing code between legacy and era-based modules (run commands, CLI parsers, errors etc)
• Make it easier to migrate code from legacy to era-based modules

The first two goals are important for the eventual day we remove legacy modules from the project.

If we proceed along these goals, it should be possible to delete just the legacy modules when the time comes and break (at compile time) very little code.

If era-based modules depend on legacy modules, the process will be a lot more involved.

Furthermore, if these boundary is not established early then more code will be written that causes the bad dependency to become more and more entrenched. Haskell does not allow cyclic dependencies after all, so there is a risk more and more code gets put into legacy modules as has already happened and used from era-based modules and existing bad dependencies will prevent us from adding newer dependencies that point in the right direction.

The third goal means we duplicate code less and if we fix bugs, we will not need to fix them in two places.

The fourth goal is important because there are existing commands in the legacy command structure we ought to port over to the era-based command structure (transaction for instance) and in the longer term we want to deprecate and delete the legacy command structure so porting is necessary.

The module structure that results from this PR is as follows:

The module structure for EraBased commands:

• Cardano.CLI.EraBased.Commands.**
  Command types and render functions
• Cardano.CLI.EraBased.Errors.**
  Errors
• Cardano.CLI.EraBased.Options.Common
  CLI parsers for non-command types
• Cardano.CLI.EraBased.Options.**
  CLI parsers for commands
• Cardano.CLI.EraBased.Run.**
  Command run functions

The module structure for Legacy commands follows the same pattern except under Cardano.CLI.Legacy instead.

It is not anticipated that this PR will affect ongoing work (much) because it deals with the relationship between legacy code and era-based code and moving legacy code around.

Feature work is currently concentrated around era-based modules that are already largely where they are supposed to be and this PR does not change that.

Reading this PR

This PR is admittedly very large. However, in order to achieve the stated aims, a lot of code needs to move around.

The risk of this PR is minimised by the fact that no code is new (this is just a matter of moving code around after all) and the fact we have CLI golden tests which serve as a reliable regression test for the CLI interface.

There are no changes to *.cli files which means the CLI interface has not changed at all.

Every commit in this PR compiles.

This means if necessary, it is possible to review the commits of the PR individually in chronological order. If there are issues with merging this PR in its entirety it is possible to select a sequence of commits that can be merged instead and the rest pushed to a new PR.

If this is the case, when reviewing this PR, indicate which commit you are willing to merge up too as part of this PR and the PR can be reset to that commit and a new PR raised for the rest.

All of the commits are designed to service the four goals listed.

If a module was renamed, it was because it did not conformed to the desired separation.

If a module was split, it was because of one of the following:

• It was a large module which if cut into smaller pieces would make migrating commands into era-based easier
• It contained both era-based code and legacy code and needed to be split
• It contained shared code which needed to be moved to era-based or to some common location so the bad dependency from era-based to legacy never happens

If modules are merged, it was because the code they contained serve a similar purpose.

An additional change was made which is that all legacy command type names are prefix with Legacy. The intention of this is to drop the EraBased prefix from era-based command types. A prefix is necessary to avoid confusion between the two types (which is possible). It is better that it is the legacy code is prefixed rather than the era-based code because we will increasingly work with era-based code so it is better for the naming overhead to live with the legacy code.

Related PRs

A lot of the changes were inspired by #158 which involves importing transaction build into the era-based command structure. The move of that command was not straight forward due to modules not being in the right place and import direction being wrong and inducing cycling dependencies among other things.

The idea with this PR is to collect all those changes that are "dumb refactoring" into one PR and apply it across the board (rip the bandaid so-to-speak).

Int is anticipated that after merging this PR, that other PR will be a lot smaller.

Verification

It was verified no EraBased modules depend on Legacy modules like this:

find cardano-cli/src/Cardano/CLI/EraBased -name '*.hs' | grep -v dist-newstyle | xargs grep Legacy

If the command returns no results, it means no bad dependencies are present.

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
• 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
• The changelog section in the PR is updated to describe the change
• Self-reviewed the diff

[carbolymer]

I think I dont't get this part:

An additional change was made which is that all legacy command type names are prefix with Legacy. The intention of this is to drop the EraBased prefix from era-based command types. A prefix is necessary to avoid confusion between the two types (which is possible). It is better that it is the legacy code is prefixed rather than the era-based code because we will increasingly work with era-based code so it is better for the naming overhead to live with the legacy code.

EraBased commands aren't prefixed from what I could find.

I'm not a big fan of prefixing types with Legacy - module name should be enough imo. Unless we want to turn types into a constant reminder that they should be removed eventually.

[newhoggy]

One of the reasons I opted for Legacy vs just L or omitting the prefix is greppability.

For example I know the type hasn't been imported indirectly into an EraBased module via some other module without Legacy in its qualified name:

find cardano-cli/src/Cardano/CLI/EraBased -name '*.hs' | grep -v dist-newstyle | xargs grep Legacy

This is also an additional reason why it's better to prefix the legacy code than the era-based code.

[carbolymer]

The module name is Legacy so without prefixing types you should be able to do the same, because the import would be there, right?

[newhoggy]

That is true, but imports can still pass transitively through modules that lack Legacy in the qualified module name.

[newhoggy]

The module name is the big one though, so greppability of the prefix is a secondary concern in comparision, yes.

[newhoggy]

The surest way is to put it in a different library component altogether, but not sure if we are keen on going that far.

[carbolymer]

Additionally, you can use HLS call-hierarchy to inspect usage sites. I think I also saw one team somewhere which had CI checks for modules' imports limiting what could be included from where - I think this should be doable in haskell as well.

In the end I'm not pushing for changing it to L, I'm just listing possible alternatives. If you feel like leaving this change as is, I'm ok with it. 😃

[newhoggy]

It will be routine to import EraBased modules from Legacy modules so we could drop the Legacy prefix altogether at the cost of using qualified imports or EraBased code when in a Legacy module.

[newhoggy]

Let's leave it in there and have a separate PR for deciding what to do with the prefix. That way we can have a good look at the impact of different decisions with actual code.

[Jimbo4350]

I prefer the Legacy prefix, it immediately tells me I'm dealing with a legacy command (i.e I don't have to think about it).

[carbolymer]

Suggestion: prefix constructors with L instead of Legacy for better readability. The meaning still would be obvious enough I think.

[carbolymer]

Good effort!

[Jimbo4350]

Nice work doing this now and saving us more headache down the road 👍

Cardano.Cli.* has some top level modules that we should also group together/move in a more appropriate place

[Jimbo4350]

👍

[newhoggy]

Ok cool. I'll do a follow up.