Rename `FeatureInEra` to `Eon` #247

newhoggy · 2023-09-16T06:28:05Z

Changelog

- description: |
    - Rename `FeatureInEra` to `Eon`
    - Rename the following modules:
      - `Cardano.Api.Feature.AlonzoEraOnly -> Cardano.Api.Eon.AlonzoEraOnly`
      - `Cardano.Api.Feature.AlonzoEraOnwards -> Cardano.Api.Eon.AlonzoEraOnwards`
      - `Cardano.Api.Feature.BabbageEraOnwards -> Cardano.Api.Eon.BabbageEraOnwards`
      - `Cardano.Api.Feature.ConwayEraOnwards -> Cardano.Api.Eon.ConwayEraOnwards`
      - `Cardano.Api.Feature.ShelleyToAllegraEra -> Cardano.Api.Eon.ShelleyToAllegraEra`
      - `Cardano.Api.Feature.ShelleyToAlonzoEra -> Cardano.Api.Eon.ShelleyToAlonzoEra`
      - `Cardano.Api.Feature.ShelleyToBabbageEra -> Cardano.Api.Eon.ShelleyToBabbageEra`
      - `Cardano.Api.Feature.ShelleyToMaryEra -> Cardano.Api.Eon.ShelleyToMaryEra`
    - Rename the following functions:
      - `inEraFeature` to `inEonEra`
      - `inEraFeature` to `eraInEon`
    - Rename the following functions (conservatively replacing "feature" with "eon" until we have a better naming convention):
      - `inEraFeatureMaybe -> inEraEonMaybe`
      - `maybeFeatureInEra -> maybeEonInEra`
      - `featureInShelleyBasedEra -> eonInShelleyBasedEra`
      - `inShelleyBasedEraFeature -> inShelleyBasedEraEon`
      - `inShelleyBasedEraFeatureMaybe -> inShelleyBasedEraEonMaybe`
      - `maybeFeatureInShelleyBasedEra -> maybeEonInShelleyBasedEra`
  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

The types that we have so far called "features" do not seem like features at all. Rather they represent some set of eras that scope the applicability of certain functionality.

This PR renames them to be "eons" instead. An Eon is some subset of eras.

Checklist

carbolymer · 2023-09-18T09:48:08Z

cardano-api/internal/Cardano/Api/Eras/Core.hs

+-- | An Eon is a span of multiple eras.  Eon's are used to scope functionality to
+-- particular eras such that it isn't possible construct code that uses functionality
+-- that is not available given eras.
+class IsEon (eon :: Type -> Type) where


Clever name. 🙂 I guess we can rename era ranges into e.g. AlonzoOnwardsEon to highlight that those are eons.

I was thinking of dropping the suffix altogether so that it is just AlonzoOnwards but I would do it as a separate change.

This is definitely a more appropriate name but instead of IsEon why not InEon and inEon?
The comment could read: Determine the value to use in a span of eras (eon)

Eon is unclaimed so it can be just that.

The instances would look like this:

instance Eon AlonzoOnwards where ...

carbolymer · 2023-09-18T09:55:35Z

cardano-api/internal/Cardano/Api/ProtocolParameters.hs

@@ -1029,8 +1029,8 @@ data ProtocolUTxOCostPerWordFeature era where
 deriving instance Eq   (ProtocolUTxOCostPerWordFeature era)
 deriving instance Show (ProtocolUTxOCostPerWordFeature era)

-instance FeatureInEra ProtocolUTxOCostPerWordFeature where
-  featureInEra no yes = \case
+instance IsEon ProtocolUTxOCostPerWordFeature where


From the naming point of view, I think this is confusing. Feature isn't an eon. If we want to use the same class for that, I'd suggest using more universal name e.g. IsAvailableInEra or IsPresentInEra or HasWitnessInEra.

ProtocolUTxOCostPerWordFeature could actually be replaced with AlonzoEraOnly because they are isomorphic to each other:

data ProtocolUTxOCostPerWordFeature era where ProtocolUpdateUTxOCostPerWordInAlonzoEra :: ProtocolUTxOCostPerWordFeature AlonzoEra

vs

data AlonzoEraOnly era where AlonzoEraOnlyAlonzo :: AlonzoEraOnly AlonzoEra

and the latter has good integration with era feature switching functions and constraints summoning whereas the former is just the datatype.

All the feature GADTs can actually be replaced in this manner so the overloaded use of the name should not arise.

Makes sense. If we won't have to use this class for features I'm all for this change.

cardano-api/internal/Cardano/Api/Eon.hs

carbolymer

Nice name, but I don't think it fits our case in 100% https://github.com/input-output-hk/cardano-api/pull/247/files#r1328491918
@Jimbo4350 what do you think?

Jimbo4350 · 2023-09-18T13:09:10Z

cardano-api/internal/Cardano/Api/Eras/Core.hs

+-- | An Eon is a span of multiple eras.  Eon's are used to scope functionality to
+-- particular eras such that it isn't possible construct code that uses functionality
+-- that is not available given eras.
+class IsEon (eon :: Type -> Type) where


This is definitely a more appropriate name but instead of IsEon why not InEon and inEon?
The comment could read: Determine the value to use in a span of eras (eon)

cardano-api/internal/Cardano/Api/Eras/Core.hs

cardano-api/internal/Cardano/Api/Eon/AlonzoEraOnly.hs

cardano-api/gen/Test/Gen/Cardano/Api/Typed.hs

Comments addressed

Jimbo4350 · 2023-09-20T13:22:59Z

cardano-api/internal/Cardano/Api/Eon.hs

+import           Cardano.Api.Eras.Core
+
+-- | A value only if the eon includes era
+data Featured eon era a where


currentEra or targetEra instead of era?

It's the same sort of era as as the era in ShelleyBasedEra era in that we don't say ShelleyBasedEra currentEra. The qualification seems unnecessary.

Jimbo4350 · 2023-09-20T13:24:10Z

cardano-api/internal/Cardano/Api/Eras/Core.hs

-- but not others.
-class FeatureInEra (feature :: Type -> Type) where
-  -- | Determine the value to use for a feature in a given 'CardanoEra'.
+-- | An Eon is a span of multiple eras.  Eon's are used to scope functionality to


Jimbo4350 · 2023-09-20T14:00:10Z

cardano-api/internal/Cardano/Api/Eras/Core.hs

-- | Determine the value to use for a feature in a given 'ShelleyBasedEra'.
-featureInShelleyBasedEra :: ()
-  => FeatureInEra feature
+  inEonEra :: ()


inEonForEra? inEonEra is shorter but it's confusing to me

I adopted inEonForEra and also forEraInEon for the other function where CardanoEra era is the first argument.

carbolymer

I think current naming is confusing, but If you prefer the current state, feel free to merge as it is now.

carbolymer · 2023-09-20T14:47:42Z

cardano-api/internal/Cardano/Api/Eras/Core.hs

+inEraEonMaybe :: ()
+  => Eon eon
+  => CardanoEra era   -- ^ Era to check
+  -> (eon era -> a)   -- ^ Function to get the value to use if the eon includes the era
+  -> Maybe a          -- ^ The value to use
+inEraEonMaybe era yes =
+  eraInEon era Nothing (Just . yes)


Suggested change

inEraEonMaybe :: ()

=> Eon eon

=> CardanoEra era -- ^ Era to check

-> (eon era -> a) -- ^ Function to get the value to use if the eon includes the era

-> Maybe a -- ^ The value to use

inEraEonMaybe era yes =

eraInEon era Nothing (Just . yes)

whenEraInEon :: ()

=> Eon eon

=> CardanoEra era -- ^ Era to check

-> (eon era -> a) -- ^ Function to get the value to use if the eon includes the era

-> Maybe a -- ^ The value to use

whenEraInEon era yes =

eraInEon era Nothing (Just . yes)

also Alternative f could be used instead of Maybe everywhere

carbolymer · 2023-09-20T15:00:37Z

cardano-api/internal/Cardano/Api/Eras/Core.hs

+maybeEonInEra :: ()
+  => Eon eon
+  => CardanoEra era   -- ^ Era to check
+  -> Maybe (eon era)  -- ^ The eon if supported in the era
+maybeEonInEra =


Suggested change

maybeEonInEra :: ()

=> Eon eon

=> CardanoEra era -- ^ Era to check

-> Maybe (eon era) -- ^ The eon if supported in the era

maybeEonInEra =

eonHasEraMaybe :: ()

=> Eon eon

=> CardanoEra era -- ^ Era to check

-> Maybe (eon era) -- ^ The eon if supported in the era

eonHasEraMaybe =

carbolymer · 2023-09-20T15:01:54Z

cardano-api/internal/Cardano/Api/Eras/Core.hs


-maybeFeatureInShelleyBasedEra :: ()
-  => FeatureInEra feature
+maybeEonInShelleyBasedEra :: ()


Suggested change

maybeEonInShelleyBasedEra :: ()

whenShelleyBasedEraInEon :: ()

carbolymer · 2023-09-20T15:09:47Z

cardano-api/internal/Cardano/Api/Eras/Core.hs


-inShelleyBasedEraFeature :: ()
-  => FeatureInEra feature
+inShelleyBasedEraEon :: ()


Suggested change

inShelleyBasedEraEon :: ()

shelleyBasedEraInEon :: ()

carbolymer · 2023-09-20T15:10:38Z

cardano-api/internal/Cardano/Api/Eras/Core.hs

+  inEonEra Nothing Just
+
+-- | Determine the value to use for a eon in a given 'ShelleyBasedEra'.
+eonInShelleyBasedEra :: ()


this function is just inShelleyBasedEraEon with its arguments reordered. I think it is redundant and inShelleyBasedEraEon can be used everywhere instead.

carbolymer · 2023-09-20T15:11:41Z

cardano-api/internal/Cardano/Api/Eras/Core.hs


-inShelleyBasedEraFeatureMaybe :: ()
-  => FeatureInEra feature
+inShelleyBasedEraEonMaybe :: ()


Suggested change

inShelleyBasedEraEonMaybe :: ()

whenShelleyBasedEraInEon :: ()

carbolymer · 2023-09-20T15:12:32Z

cardano-api/internal/Cardano/Api/Eras/Core.hs

-- | Determine the value to use for a feature in a given 'ShelleyBasedEra'.
-featureInShelleyBasedEra :: ()
-  => FeatureInEra feature
+  inEonEra :: ()


Suggested change

inEonEra :: ()

hasEra :: ()

or hasEonEra or eraInEon

I adopted this suggestion: #247 (comment)

cardano-api/internal/Cardano/Api/Eras/Core.hs

carbolymer · 2023-09-20T15:17:31Z

cardano-api/internal/Cardano/Api/Eras/Core.hs

+-- | An Eon is a span of multiple eras.  Eon's are used to scope functionality to
+-- particular eras such that it isn't possible construct code that uses functionality
+-- that is not available given eras.
+class Eon (eon :: Type -> Type) where


Suggested change

class Eon (eon :: Type -> Type) where

class IsEon (eon :: Type -> Type) where

I feel that using IsEon instead of plain noun would be better suited here and represent the fact that something is an eon. I'd reserve plain nouns for concrete datatypes, just in case.

I'm okay with IsEon or Eon. I don't think there is a strong case for either. The standard library actually uses both.

For example the standard library uses Generic instead of IsGeneric whilst it uses IsString instead of String. I think the Is in IsString is necessary because the type String already exists, but we don't use IsGeneric and use Generic instead because there isn't a Generic type so the name is available for the type class.

newhoggy · 2023-09-20T15:59:25Z

I think current naming is confusing, but If you prefer the current state, feel free to merge as it is now.

I don't actually prefer the current wording. I think the current wording is sub-optimal.

Nevertheless the goal of the PR isn't to optimise the naming of these functions. It's to remove the word "feature" because it isn't appropriate for the abstraction.

I'm merging this PR as it is to unblock other work.

We can have a separate PR to sort out the naming.

…ay-1.8 Update cardano-cli to newer cardano-ledger

newhoggy marked this pull request as ready for review September 16, 2023 06:28

newhoggy requested review from dcoutts, Jimbo4350, erikd, carbolymer, CarlosLopezDeLara and disassembler as code owners September 16, 2023 06:28

carbolymer reviewed Sep 18, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Eon.hs Outdated Show resolved Hide resolved

carbolymer reviewed Sep 18, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/rename-feature-to-eon branch from cba294a to 3264c3a Compare September 18, 2023 12:13

Jimbo4350 reviewed Sep 18, 2023

View reviewed changes

carbolymer reviewed Sep 18, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Eras/Core.hs Outdated Show resolved Hide resolved

newhoggy force-pushed the newhoggy/rename-feature-to-eon branch 5 times, most recently from a44dc7e to a563862 Compare September 18, 2023 23:51

github-advanced-security bot found potential problems Sep 18, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Eon/AlonzoEraOnly.hs Fixed Show fixed Hide fixed

newhoggy force-pushed the newhoggy/rename-feature-to-eon branch 2 times, most recently from ce64aad to 412242a Compare September 19, 2023 01:54

newhoggy requested review from carbolymer and Jimbo4350 September 19, 2023 01:55

Jimbo4350 previously requested changes Sep 19, 2023

View reviewed changes

cardano-api/gen/Test/Gen/Cardano/Api/Typed.hs Outdated Show resolved Hide resolved

Fix typo

500b62e

newhoggy force-pushed the newhoggy/rename-feature-to-eon branch 2 times, most recently from 9853461 to 822425f Compare September 20, 2023 07:18

newhoggy requested a review from Jimbo4350 September 20, 2023 09:41

Jimbo4350 reviewed Sep 20, 2023

View reviewed changes

carbolymer approved these changes Sep 20, 2023

View reviewed changes

Rename Feature module to Eon

072d0bf

newhoggy force-pushed the newhoggy/rename-feature-to-eon branch from 822425f to da43ac2 Compare September 20, 2023 15:32

newhoggy added 4 commits September 21, 2023 01:55

Rename FeatureInEra to Eon

4294968

Rename featureInEra to inEonForEra

08f0e3a

Rename inEraFeature to forEraInEon

ff7e3a7

Rename Feature to Eon in eon related functions

9469db1

newhoggy force-pushed the newhoggy/rename-feature-to-eon branch from da43ac2 to 9469db1 Compare September 20, 2023 15:55

Update cabal-cache version

e4e590d

newhoggy requested a review from a team as a code owner September 20, 2023 16:02

newhoggy enabled auto-merge September 20, 2023 16:03

Jimbo4350 approved these changes Sep 20, 2023

View reviewed changes

newhoggy added this pull request to the merge queue Sep 20, 2023

Merged via the queue into main with commit ee39ce5 Sep 20, 2023
20 checks passed

newhoggy deleted the newhoggy/rename-feature-to-eon branch September 20, 2023 19:10

newhoggy pushed a commit that referenced this pull request Mar 11, 2024

Merge pull request #247 from input-output-hk/coot/cardano-ledger-conw…

9fb8347

…ay-1.8 Update cardano-cli to newer cardano-ledger

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `FeatureInEra` to `Eon` #247

Rename `FeatureInEra` to `Eon` #247

newhoggy commented Sep 16, 2023 •

edited

Loading

carbolymer Sep 18, 2023

newhoggy Sep 18, 2023 •

edited

Loading

Jimbo4350 Sep 18, 2023

newhoggy Sep 18, 2023 •

edited

Loading

carbolymer Sep 18, 2023 •

edited

Loading

newhoggy Sep 18, 2023 •

edited

Loading

carbolymer Sep 18, 2023

carbolymer left a comment

Jimbo4350 Sep 18, 2023

Jimbo4350 Sep 20, 2023

newhoggy Sep 20, 2023 •

edited

Loading

Jimbo4350 Sep 20, 2023

Jimbo4350 Sep 20, 2023

newhoggy Sep 20, 2023

carbolymer left a comment •

edited

Loading

carbolymer Sep 20, 2023

carbolymer Sep 20, 2023

carbolymer Sep 20, 2023

carbolymer Sep 20, 2023

carbolymer Sep 20, 2023

carbolymer Sep 20, 2023

carbolymer Sep 20, 2023

newhoggy Sep 20, 2023

carbolymer Sep 20, 2023

newhoggy Sep 20, 2023

newhoggy commented Sep 20, 2023 •

edited

Loading

	maybeEonInShelleyBasedEra :: ()
	whenShelleyBasedEraInEon :: ()

	inShelleyBasedEraEonMaybe :: ()
	whenShelleyBasedEraInEon :: ()

	class Eon (eon :: Type -> Type) where
	class IsEon (eon :: Type -> Type) where

Rename FeatureInEra to Eon #247

Rename FeatureInEra to Eon #247

Conversation

newhoggy commented Sep 16, 2023 • edited Loading

Changelog

Context

Checklist

Choose a reason for hiding this comment

newhoggy Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

carbolymer Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy commented Sep 20, 2023 • edited Loading

Rename `FeatureInEra` to `Eon` #247

Rename `FeatureInEra` to `Eon` #247

newhoggy commented Sep 16, 2023 •

edited

Loading

newhoggy Sep 18, 2023 •

edited

Loading

newhoggy Sep 18, 2023 •

edited

Loading

carbolymer Sep 18, 2023 •

edited

Loading

newhoggy Sep 18, 2023 •

edited

Loading

newhoggy Sep 20, 2023 •

edited

Loading

carbolymer left a comment •

edited

Loading

newhoggy commented Sep 20, 2023 •

edited

Loading