Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CDEC-432] Move Mockable and JsonLog to core #3238

Merged
merged 3 commits into from
Jul 13, 2018

Conversation

mhuesch
Copy link
Contributor

@mhuesch mhuesch commented Jul 12, 2018

Description

Move Mockable and JsonLog into core, so that networking can be freed up. The goal is to let networking float upwards in the dependency graph.

Linked issue

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

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.
    ^ this PR moves code around & doesn't change functionality. Existing tests should be good enough.





Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that is a lot of empty lines. Is that just the way it was in the original file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that was indeed the case. Weird!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep 🤷‍♂️ In the spirit of minimal changes I left it. But several of these modules definitely could use tidying up.

@erikd
Copy link
Member

erikd commented Jul 13, 2018

As you can see from the dependency graph, the networking package is in this werid place at the bottom of the graph, with nearly everything else depending on it.

This is why there is also networking code in infra and lib. By moving Mockable and the JsonLog stuff to core, networking can be moved up in the graph. After this commit is merged we would then work up the dependency graph replacing the networking versions of Mockable and the JsonLog with the core version until nothing more depends on the networking versions and we can then drop the re-exports.

We do it like this so the transition happens in a number of smaller PRs that are easier to review (this one is by far the worst).

It should also be noted that the dependency graph above is about a week out-of-date and that infra has already been moved up near cardano-sl. When networking is also moved up the graph we would like to move all the networking code to a single package.

Michael Hueschen added 3 commits July 13, 2018 14:09
These modules were previously in `networking`, which caused a
dependency from `core` on `networking`. In an effort to free up
`networking` (so that it can move up in the dep graph), we move
those 2 modules into `core`.

Several of the moved modules relied on an implicit prelude in
`networking`. Since `core` has NoImplicitPrelude in its
`default-extensions`, we had to add `import Prelude`.

We also re-export `JsonLog` & `Mockable` from `networking`.
@mhuesch mhuesch force-pushed the mhuesch/CDEC-432-1-squashed branch from 50ef0e1 to 0aeeae9 Compare July 13, 2018 18:10
Copy link
Member

@erikd erikd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I happy with this as it is and @mhuesch has communicated with @avieth who also seems ok with this plan of action but Alex has not as far as I am aware looked at his PR.

@mhuesch mhuesch merged commit 936e609 into develop Jul 13, 2018
@mhuesch mhuesch deleted the mhuesch/CDEC-432-1-squashed branch July 13, 2018 22:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants