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

Move command errors from era-based to CmdError module #188

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Aug 18, 2023

Changelog

- description: |
   Move command errors from era-based to `CmdError` module
# 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

It is often better for errors to live in a module separate from other code.

For command errors in particular this is especially true.

This is because sometimes moving a function that has a error type in its signature, if that function were defined in the same module as the error type and that function needed to move, it is very often the case that the error type has to move with it.

This is especially true with commands that have a strict hierarchical structure and you wanted to push down a command into a subgroup.

If runX throws Error and they both live in Parent and you want to move runX to Child that requires you to move the Error type to Child as well.

If you have runX throws Error and runY throws Error and you wanted to move runX to ChildX and runY to ChildY, Error can't move with both of them and you're stuck with having to do a more drastic change.

In this case you're forced to move Error to a shared module or split the Error type.

Perhaps that is one of these is the proper approach, but it doesn't change the fact that a simple move can become a more complicated change and PRs become bigger than we want them to be.

If the Error types were in a separate module to start with the problem doesn't arise.

Over the course of Conway work, there will be a lot of restructuring of commands. Having command error types live with command functions will be very disruptive to this process and it is better to just move them out of the way to cause less pain going forward.

This PR moves these error types into CmdError, this does not mean this is the most ideal home for them. Just that they are not co-inhabiting with command functions and so not be in a position of triggering the bad situation.

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

@newhoggy newhoggy marked this pull request as ready for review August 18, 2023 16:37
@newhoggy newhoggy added this pull request to the merge queue Aug 18, 2023
Merged via the queue into main with commit f2b12c5 Aug 18, 2023
24 of 28 checks passed
@newhoggy newhoggy deleted the newhoggy/move-command-errors-CmdError-module branch August 18, 2023 17:04
@carbolymer
Copy link
Contributor

If you have runX throws Error and runY throws Error and you wanted to move runX to ChildX and runY to ChildY, Error can't move with both of them and you're stuck with having to do a more drastic change.

What do you think about going in the direction of splitting the errors into separate types to avoid sharing error types between runX and runY?

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.

None yet

3 participants