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.

Sign up for GitHub

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

Jump to bottom

Expose CurrentEra and UpcomingEra pattern synonyms #414

Merged

Jimbo4350 merged 1 commit into main from jordan/era-handling-refactor-2

Dec 22, 2023

Contributor

Jimbo4350 commented Dec 21, 2023 •

edited

Loading

@carbolymer pointed out the following situation and suggested the use of pattern synonyms to remedy it.

Changelog

- description: |
    Expose CurrentEra and UpcomingEra pattern synonyms
# 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

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

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 requested review from dcoutts, erikd, newhoggy, carbolymer, CarlosLopezDeLara and disassembler as code owners

December 21, 2023 18:04

Jimbo4350 mentioned this pull request

Era handling #402

Merged

3 tasks

Jimbo4350 changed the title ~~Expose CurrentEra and UpcomingEra pattern synonyms~~ WIP: Expose CurrentEra and UpcomingEra pattern synonyms

Jimbo4350 changed the title ~~WIP: Expose CurrentEra and UpcomingEra pattern synonyms~~ Expose CurrentEra and UpcomingEra pattern synonyms

Jimbo4350 force-pushed the jordan/era-handling-refactor-2 branch from 202cbf2 to 556450b Compare

December 21, 2023 18:17

github-advanced-security bot found potential problems

View reviewed changes

cardano-api/internal/Cardano/Api/Protocol/Version.hs Fixed Show fixed Hide fixed

Jimbo4350 force-pushed the jordan/era-handling-refactor-2 branch 4 times, most recently from 29c1603 to 0fbb6fb Compare

December 21, 2023 18:29

Jimbo4350 commented

View reviewed changes

cardano-api/internal/Cardano/Api/Protocol/Version.hs Outdated

+              --   doThing :: Era era -> ()
+              --   doThing = \case
+              --     CurrentEra' -> enableFeature
+              --     UpcomingEra' -> disableFeature

Contributor Author

Jimbo4350 Dec 21, 2023 •

edited

Loading

Add comment elaborating on why this is undesirable i.e updating the type synonyms will go unnoticed by consumers. Also include comment in this module explaining how we will deprecate eras.

carbolymer reviewed

View reviewed changes

cardano-api/internal/Cardano/Api/Protocol/Version.hs Outdated

-                CurrentEra
-                  :: SupportedProtocolVersionRange BabbageEra
-                  => Era BabbageEra
+                CurrentEra'

Contributor

carbolymer Dec 21, 2023

I think being clear that this constructor should not be used would be better than just apostrophe.

Suggested change

      
              CurrentEra'
          
              UnsafeCurrentEra

Contributor Author

Jimbo4350 Dec 22, 2023

Unsafe implies something breaks the type system, memory safety etc. I'll append Internal to the constructors

carbolymer approved these changes

View reviewed changes

Jimbo4350 force-pushed the jordan/era-handling-refactor-2 branch 3 times, most recently from 045f9f2 to 3cd3610 Compare

December 22, 2023 14:05

Jimbo4350 enabled auto-merge

December 22, 2023 14:07

Jimbo4350 force-pushed the jordan/era-handling-refactor-2 branch from 3cd3610 to 442b5c8 Compare

December 22, 2023 14:29

Jimbo4350 disabled auto-merge

December 22, 2023 14:32

Jimbo4350 force-pushed the jordan/era-handling-refactor-2 branch 2 times, most recently from 721fd66 to 74966c0 Compare

December 22, 2023 14:33


          Expose CurrentEra and UpcomingEra pattern synonyms

e999814

Jimbo4350 force-pushed the jordan/era-handling-refactor-2 branch from 74966c0 to e999814 Compare

December 22, 2023 14:34

Jimbo4350 added this pull request to the merge queue

Merged via the queue into main with commit 1c10a28

20 checks passed

Jimbo4350 deleted the jordan/era-handling-refactor-2 branch

December 22, 2023 15:02

carbolymer reviewed

View reviewed changes

cardano-api/internal/Cardano/Api/Protocol/Version.hs

+. Add new 'UseEra' instance and keep the deprecated era's instance.
+              @
+              instance UseEra BabbageEra where

Contributor

carbolymer Dec 22, 2023

I think we can add also a TypeError here e.g.:

instance TypeError (Text "BabbageEra no longer supported, use ConwayEra") => UseEra BabbageEra where

Contributor

carbolymer Dec 22, 2023

...or maybe not. I guess the point is to have the project buildable, but also generate a deprecation warning.

carbolymer reviewed

View reviewed changes

cardano-api/internal/Cardano/Api/Protocol/Version.hs

+. Add new 'UseEra' instance and keep the deprecated era's instance.
+              @
+              instance UseEra BabbageEra where

Contributor

carbolymer Dec 22, 2023

...or maybe not. I guess the point is to have the project buildable, but also generate a deprecation warning.

cardano-api/internal/Cardano/Api/Protocol/Version.hs

-                  ( MinSupportedVersion <= version
-                  , version <= MaxSupportedVersion
-                  )
+              data BabbageEra

Contributor

carbolymer Dec 22, 2023

What's the "lifecycle" of uninhabited era types? After forking to conway, will we remove the old era, or leave it for some time?

newhoggy added a commit that referenced this pull request


          Merge pull request #414 from input-output-hk/newhoggy/command-argumen…

d168465

…t-types-for-poll-commands

Command argument types for poll commands

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

carbolymer carbolymer approved these changes

dcoutts Awaiting requested review from dcoutts

erikd Awaiting requested review from erikd

newhoggy Awaiting requested review from newhoggy

CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

disassembler Awaiting requested review from disassembler

Labels

None yet