Review folder structure, file documentation and pragmas #37

KtorZ · 2019-03-07T13:59:32Z

Issue Number

#20

Overview

Review haddock documentation for Cardano.Wallet.Binary
Remove unnecessart intermediary modules 'BinaryHelpers' and 'Binary/Helpers'
Rename and organize test data to reflect source code organization
Add license and copyright to source files' header

Comments

I've split the previous PR (#34 into two, and kept #34 really just about the tx id)

Anviking

Nice 👍

One concrete naming-suggestion, and a some non-important questions.

Also, looks like you both moved and modified code-sections in the same commit. I find it practical to split up into two commits:

Move / reorder
Modify

for readability.

Would't have minded one commit per bullet-point either. Here it would have accomplished the reorder-modify-separation at the same time.

src/Cardano/Wallet/Binary.hs

Anviking · 2019-03-07T18:46:19Z

src/Cardano/Wallet/Binary.hs

        index <- CBOR.decodeWord32
-        return $ TxIn (Hash txId) index


🤔 I guess the reasoning here is that the id part of txId is obvious.

I think I'd prefer

tx <- Hash <$> CBOR.decodeBytes ... return $ TxIn tx index

to make it obvious that we're not hashing a tx, but that tx is a Hash.

Done ( https://github.com/input-output-hk/cardano-wallet/compare/ebe31ebbfc96a055b44ae3da687aa6eef6b939d5..7a6593d11547b4be6abd5752dc4cea96f002a507 )

Anviking · 2019-03-07T17:36:33Z

src/Cardano/Wallet/Binary.hs

@@ -376,3 +385,49 @@ decodeUpdateProof :: CBOR.Decoder s ()
 decodeUpdateProof = do
    _ <- CBOR.decodeBytes -- Update Hash
    return ()
+
+
+-- * Helpers


Anviking · 2019-03-07T18:05:36Z

src/Cardano/Wallet/Binary.hs

-- trusted node and therefore, we needn't to care about verifying signatures and
-- blocks themselves).
+-- |
+-- Copyright: © 2018-2019 IOHK


I assume this is good, and for when an individual file is included in another project.

Other IOHK projects doesn't seem to have this. Any particular reason for why we/now?

This is actually a standard practice in Haskell modules that are exported. This get parsed by haddock to generate a header for each module.

Anviking · 2019-03-07T18:38:41Z

src/Cardano/Wallet/Binary.hs

 decodeBlock :: CBOR.Decoder s Block
 decodeBlock = do
    CBOR.decodeListLenCanonicalOf 2
    t <- CBOR.decodeWordCanonical
    case t of
        0 -> do -- Genesis Block
            _ <- CBOR.decodeListLenCanonicalOf 3
-            header <- decodeGenesisBlockHeader
+            h <- decodeGenesisBlockHeader


🤔 sure…

Yeah, there was a hlint annotation at the root of the file asking to "ignore name shadowing", I removed it and adjusted a few names to get rid of the warnings.

Anviking · 2019-03-07T18:58:46Z

test/unit/Cardano/Wallet/Binary/PackfileSpec.hs

@@ -34,7 +34,7 @@ testTwoBlobs = packFileHeader
 -- Get this file from cardano-http-bridge with:
 -- wget -O test/data/epoch-mainnet-104 http://localhost:8080/mainnet/epoch/104
 testPackfile :: FilePath
-testPackfile = "test/data/epoch-mainnet-104"
+testPackfile = "test/data/Cardano/Wallet/Binary/PackfileSpec-epoch-mainnet-104"


👍 I'm starting to wonder whether there are editor plugins for browsing the shared structure

🤷‍♂️

KtorZ · 2019-03-08T08:44:16Z

looks like you both moved and modified code-sections in the same commit. I find it practical to split up into two commits

@Anviking ^^^ Yeah, it was the case originally and when I did the split of this PR from #34, I did squash everything in one commit. I don't know why :/ ... Your remark is very sensible though -- sorry about that.

- Review haddock documentation for Cardano.Wallet.Binary - Remove unnecessart intermediary modules 'BinaryHelpers' and 'Binary/Helpers' - Rename and organize test data to reflect source code organization - Add license and copyright to source files' header

… declarations

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes [3] hlint suggestion

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes [3] hlint suggestion [3] add .weeder.yaml to omit duplicateMaybes and groups to be announce during weeder execution [3] undo weeder ignore file plus remove pragma and unnecessary exports

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes [3] hlint suggestion [3] add .weeder.yaml to omit duplicateMaybes and groups to be announce during weeder execution [3] undo weeder ignore file plus remove pragma and unnecessary exports [3] remove not needed imports

KtorZ self-assigned this Mar 7, 2019

KtorZ requested a review from Anviking March 7, 2019 13:59

KtorZ mentioned this pull request Mar 7, 2019

Define type primitives needed to represent a wallet #20

Closed

KtorZ force-pushed the KtorZ/#20/review-folder-organization branch from 9cb4ca3 to ebe31eb Compare March 7, 2019 14:35

Anviking approved these changes Mar 7, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/#20/review-folder-organization branch from ebe31eb to 7a6593d Compare March 8, 2019 08:49

KtorZ merged commit 28ce7dd into master Mar 8, 2019

KtorZ added a commit that referenced this pull request Mar 11, 2019

refactoring #37 | re-organize module to separate effectful logic from…

b292211

… declarations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review folder structure, file documentation and pragmas #37

Review folder structure, file documentation and pragmas #37

KtorZ commented Mar 7, 2019

Anviking left a comment

Anviking Mar 7, 2019

KtorZ Mar 8, 2019

Anviking Mar 7, 2019

Anviking Mar 7, 2019

KtorZ Mar 8, 2019

Anviking Mar 7, 2019

KtorZ Mar 8, 2019

Anviking Mar 7, 2019

KtorZ Mar 8, 2019

KtorZ commented Mar 8, 2019

Review folder structure, file documentation and pragmas #37

Review folder structure, file documentation and pragmas #37

Conversation

KtorZ commented Mar 7, 2019

Issue Number

Overview

Comments

Anviking 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

KtorZ commented Mar 8, 2019