[CDEC-451] Remove `Mockable` #3285

mhuesch · 2018-07-21T14:41:16Z

Description

The Mockable typeclass is a layer of indirection which allows code to be run in different monads without requiring changes to the monadic functions.

Mockable is apparently only executed in IO, and thus provides no benefit and a fair amount of hassle due to indirection & confusion for developers.

Remove the Mockable typeclass and associated instances.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CDEC-451

Type of change

[~] 🐞 Bug fix (non-breaking change which fixes an issue)
[~] 🛠 New feature (non-breaking change which adds functionality)
[~] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
[~] 🔨 New or improved tests for existing code
[~] ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.

Testing checklist

[~] I have added tests to cover my changes.
All new and existing tests passed.
^ I can see a good argument for tests to ensure the functions of Pos.Core.Conc are equivalent to those of Pos.Core.Mockable.Production. At present I have not added such tests, though.

mhuesch · 2018-07-21T14:43:52Z

Since there are a lot of changes here, I'd recommend going through the commits separately. The first 2 delete dead code which used Mockable. The next removes imports of Mockable from source code files. The next deletes the Mockable modules, and removes the last imports of Mockable from a few test packages.

mhuesch

Wanted to point out a couple things that need attention.

mhuesch · 2018-07-21T14:52:34Z

core/src/Pos/Core/Conc.hs

+
+-- | This function is analogous to `System.Timeout.timeout`, it's
+-- based on `Race` and `Delay`.
+-- TODO mhueschen - make sure this is equivalent to the old method


❗️Since these are important primitives, I think we should ensure equivalence between old & new implementations. I am unsure of the best way to do that, hence the TODO and this comment.

timeout certainly seems right to me.

Cool. So we will rely on QA & release tests to catch anything that might arise?

mhuesch · 2018-07-21T14:54:09Z

infra/src/Pos/Infra/Slotting/Util.hs

@@ -113,6 +113,8 @@ type MonadOnNewSlot ctx m =
 -- | Run given action as soon as new slot starts, passing SlotId to
 -- it.  This function uses Mockable and assumes consistency between
 -- MonadSlots and Mockable implementations.
+-- TODO mhueschen ^ get feedback on what this comment means and what
+-- we should do about it.


❗️I don't have good insight into this comment, and am confused by the implication that MonadSlots & Mockable implement equivalent functionality (they seem fairly different to me), so would appreciate any feedback on this.

I think the comment you refer to is only relevant for Mockable instances were the underlying monad is not IO.

Thanks, I think you're right. I found that the "simple/default implementation" uses currentTime, which (before this PR) used to be a Mockable function. So there could be alternative implementations which might not have the appropriate consistency that the comment mentions.

I'll update the comment.

mhuesch · 2018-07-21T14:55:47Z

tools/src/dbgen/Main.hs

-    askLoggerName = pure $ LoggerName "dbgen"
-    modifyLoggerName _ x = x
+-- TODO mhueschen get feedback about usage of this module and whether
+-- the instance from `Pos.Core.Conc` can eclipse this.


❗️I don't have insight into what this module/executable is used for, and whether it's important that the instances be the same. If it's not important, then it's simpler to keep the one instance here and delete this one.

This is in the tools package which basically just contains tools for the IOHK devops people to do what they need to do.

On top of which, the current logging implementation is being replaced soon.

Not sure if that helps.

Thanks. I'll ask in the devops channel if anyone has advice or opinions on this.

mhuesch · 2018-07-21T14:58:41Z

core/src/Pos/Core/Conc.hs

+-- | Temporary holding bin for useful functions defined in `Pos.Core.Mockable`.
+-- Since we are removing the `Mockable` class, some of the functions defined
+-- there will move here.
+module Pos.Core.Conc


❗️I created this module to avoid laborious changes to the imports of modules which had previously used Mockable (and the functions exposed from Mockable.Production). Much of the functionality exported here comes from UnliftIO or Control.Concurrent, so we could also dismantle this module and would-be importers use UnliftIO directly. I think this module is a fairly clean way to encapsulate the functionality, though.

Makes sense!

mhuesch · 2018-07-21T15:02:07Z

networking/src/Network/Transport/ConnectionBuffers.hs

-  | DuplicateConnection
-  deriving (Typeable, Show)
-
-instance Exception InternalError


It would be great to have @avieth confirm deletion of this module. It isn't included in the respective cabal file and seems fairly bitrotten.

Yeah this was just a sketch and shouldn't have been committed at all.

mhuesch · 2018-07-21T15:02:27Z

infra/src/Pos/Infra/Discovery/Model/Neighbors.hs

-            logDebug $ "converseToNeighbors: DONE conversing to node " <> show node
-    logDebug "converseToNeighbors: sending to nodes done"
-    where
-    logErr node e = logWarning $ sformat ("Error in conversation to " % shown % ": " % shown) node e


It would be great to have @avieth confirm deletion of this module. It isn't included in the respective cabal file and seems fairly bitrotten.

Yet another rotten file. Ok to delete.

erikd · 2018-07-23T00:23:04Z

This looks good, but I'd really like to get @avieth and/or @coot to have a look at this before we merge it.

Good work @mhuesch !

CodiePP · 2018-07-23T11:09:35Z

Looking forward to see this get merged into 'develop'. Getting rid of 'Production' will make our life easier!
Thanks for that great job @mhuesch

coot · 2018-07-23T12:40:57Z

LGTM, I am happy to see the change too. As long as @avieth is fine with removing the files, I am fine with it.

parsonsmatt

So much deleted code 😄

parsonsmatt · 2018-07-23T13:26:15Z

auxx/Main.hs

@@ -43,12 +42,12 @@ loggerName = "auxx"

 -- 'NodeParams' obtained using 'CLI.getNodeParams' are not perfect for
 -- Auxx, so we need to adapt them slightly.
-correctNodeParams :: AuxxOptions -> NodeParams -> Production (NodeParams, Bool)
+correctNodeParams :: AuxxOptions -> NodeParams -> IO (NodeParams, Bool)


parsonsmatt · 2018-07-23T13:28:52Z

core/examples/abuse/Main.hs

@@ -1,167 +0,0 @@
-{-# LANGUAGE BangPatterns       #-}


Why is this file deleted?

abuse used Mockable & Production, and in trying to remove Mockable from it, I realized abuse was very bitrotten and contained imports of modules deleted from cardano-sl months ago. It seemed to me that the cost of updating the code would be significant, and that deleting it might be the best call. @avieth agreed with this, and I believe he was the last one to touch abuse.

I put a comment in the git commit but should've also written an explanatory comment on the PR.

parsonsmatt · 2018-07-23T13:29:14Z

core/examples/abuse/README.md

@@ -1,177 +0,0 @@
-# Backpressure and peer abuse example


Same question, why is it deleted? Seems to not be tied to the changes in this PR.

I'm OK with removing this. It's quite rotten.

This file has occurrences of Mockable but is fairly bit-rotten. After consulting Alex Vieth, I'm deleting it.

These modules are not contained in any cabal files, and they contained occurrences of `Mockable` (and thus would become bitrotten with the new changes), so I opted to delete them.

As a first step, we remove imports of the Mockable family of modules in source code (not tests yet). We do not touch JsonLogT, which imports Mockable, yet. We also do not remove the Mockable family of modules.

avieth

Thanks for doing this!

avieth · 2018-07-23T17:27:54Z

core/examples/abuse/README.md

@@ -1,177 +0,0 @@
-# Backpressure and peer abuse example


I'm OK with removing this. It's quite rotten.

avieth · 2018-07-23T17:29:44Z

infra/src/Pos/Infra/Discovery/Model/Neighbors.hs

-            logDebug $ "converseToNeighbors: DONE conversing to node " <> show node
-    logDebug "converseToNeighbors: sending to nodes done"
-    where
-    logErr node e = logWarning $ sformat ("Error in conversation to " % shown % ": " % shown) node e


Yet another rotten file. Ok to delete.

avieth · 2018-07-23T17:30:40Z

networking/src/Network/Transport/ConnectionBuffers.hs

-  | DuplicateConnection
-  deriving (Typeable, Show)
-
-instance Exception InternalError


Yeah this was just a sketch and shouldn't have been committed at all.

CodiePP · 2018-07-23T17:47:55Z

tools/src/dbgen/Main.hs

@@ -142,10 +141,6 @@ newWalletState recreate walletPath =
    -- to rebuild the DB, but rather append stuff into it.
    liftIO $ openState (not recreate) walletPath

-instance HasLoggerName IO where


this can savely be removed, as with the new logging all 'HasLoggerName' will disappear.
a LoggerName was used as a context name in logging. We will have a new mechanism which will cover this.

Great, thanks @CodiePP.

mhuesch requested review from adinapoli-iohk, akegalj, avieth, dcoutts, denisshevchenko, erikd, ksaric and parsonsmatt as code owners July 21, 2018 14:41

mhuesch force-pushed the mhuesch/CDEC-451-sq branch from 73d8de6 to eb060b6 Compare July 21, 2018 14:46

mhuesch commented Jul 21, 2018

View reviewed changes

mhuesch force-pushed the mhuesch/CDEC-451-sq branch 2 times, most recently from 19c4b1a to 782b17f Compare July 22, 2018 16:27

parsonsmatt approved these changes Jul 23, 2018

View reviewed changes

Michael Hueschen added 5 commits July 23, 2018 11:03

[CDEC-451] Delete "abuse" example program

2246e37

This file has occurrences of Mockable but is fairly bit-rotten. After consulting Alex Vieth, I'm deleting it.

[CDEC-451] Delete dead modules

fe65ad1

These modules are not contained in any cabal files, and they contained occurrences of `Mockable` (and thus would become bitrotten with the new changes), so I opted to delete them.

[CDEC-451] Remove source-file imports of Mockable

3e48ec6

As a first step, we remove imports of the Mockable family of modules in source code (not tests yet). We do not touch JsonLogT, which imports Mockable, yet. We also do not remove the Mockable family of modules.

[CDEC-451] Remove Mockable; update test packages

1f2c8a9

[CDEC-451] Update comments / address feedback

58d7d91

mhuesch force-pushed the mhuesch/CDEC-451-sq branch from 782b17f to 58d7d91 Compare July 23, 2018 16:06

avieth approved these changes Jul 23, 2018

View reviewed changes

CodiePP reviewed Jul 23, 2018

View reviewed changes

mhuesch merged commit 58414fb into develop Jul 23, 2018

mhuesch deleted the mhuesch/CDEC-451-sq branch July 23, 2018 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CDEC-451] Remove `Mockable` #3285

[CDEC-451] Remove `Mockable` #3285

mhuesch commented Jul 21, 2018

mhuesch commented Jul 21, 2018

mhuesch left a comment

mhuesch Jul 21, 2018

erikd Jul 23, 2018

mhuesch Jul 23, 2018

mhuesch Jul 21, 2018

erikd Jul 23, 2018

mhuesch Jul 23, 2018

mhuesch Jul 21, 2018

erikd Jul 23, 2018

mhuesch Jul 23, 2018

mhuesch Jul 21, 2018

erikd Jul 22, 2018

mhuesch Jul 21, 2018

avieth Jul 23, 2018

mhuesch Jul 21, 2018

avieth Jul 23, 2018

erikd commented Jul 23, 2018

CodiePP commented Jul 23, 2018

coot commented Jul 23, 2018

parsonsmatt left a comment

parsonsmatt Jul 23, 2018

parsonsmatt Jul 23, 2018

mhuesch Jul 23, 2018

parsonsmatt Jul 23, 2018

avieth Jul 23, 2018

avieth left a comment

avieth Jul 23, 2018

avieth Jul 23, 2018

avieth Jul 23, 2018

CodiePP Jul 23, 2018

mhuesch Jul 23, 2018

[CDEC-451] Remove Mockable #3285

[CDEC-451] Remove Mockable #3285

Conversation

mhuesch commented Jul 21, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

mhuesch commented Jul 21, 2018

mhuesch left a 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

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

erikd commented Jul 23, 2018

CodiePP commented Jul 23, 2018

coot commented Jul 23, 2018

parsonsmatt left a 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

avieth left a 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

[CDEC-451] Remove `Mockable` #3285

[CDEC-451] Remove `Mockable` #3285