Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

[ethcore]: reduce re-exports #11059

Merged
merged 2 commits into from
Sep 16, 2019
Merged

[ethcore]: reduce re-exports #11059

merged 2 commits into from
Sep 16, 2019

Conversation

niklasad1
Copy link
Collaborator

Last piece of refactoring to close #10130 after work done by @dvdplm et. al

After this PR, we have the following re-exports from other crates in non-testbuilds:

  • evm::VMType (client)
  • ethcore_miner::local_accounts::LocalAccounts (miner)
  • ethcore_miner::pool::PendingOrdering (miner)

I think the miner re-exports make sense (closely tied to the module) and VMType (related to module but not closely)

Last piece of refactoring to close #10130 after work done by @dvdplm et. al

After this PR, we have the following re-exports from other crates in `non-testbuilds`:
- evm::VMType (client)
- ethcore_miner::local_accounts::LocalAccounts (miner)
- ethcore_miner::pool::PendingOrdering (miner)

I think the miner re-exports make sense (closely tied to the module) and `VMType` (related to module but not closely)
@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M4-core ⛓ Core client code / Rust. labels Sep 16, 2019
@niklasad1 niklasad1 requested a review from tomusdrw September 16, 2019 10:39
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM module minor nit.

Good stuff, I think that while reexports can be useful now and then, we're better off without them in parity-ethereum: they make it tricky to navigate the code base. Actually finding out what depends on what is non-trivial. I have it on my todo-list to see if any of the crates that currently depend on ethcore could actually be "upgraded" to use it only as a dev-dependency. I suspect that a few could (and further down my todo-list is to investigate if the test-helpers in ethcore could/should be extracted).

@ordian ordian added this to the 2.7 milestone Sep 16, 2019
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 16, 2019
@ordian ordian merged commit 0051c26 into master Sep 16, 2019
@ordian ordian deleted the na-ethcore-reduce-reexports branch September 16, 2019 15:12
dvdplm added a commit that referenced this pull request Sep 18, 2019
* master:
  [ethcore]: move client test types to test-helpers (#11062)
  [sync]: remove unused dependencies or make dev (#11061)
  [ethcore]: reduce re-exports (#11059)
  [evmbin] fix time formatting (#11060)
  Update hardcoded headers (foundation, classic, kovan,  xdai, ewc, ...) (#11053)
  cargo update -p eth-secp256k1 (#11052)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove re-exports from ethcore
4 participants