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

Remove ethcore::common re-export module #2792

Merged
merged 7 commits into from
Oct 24, 2016
Merged

Remove ethcore::common re-export module #2792

merged 7 commits into from
Oct 24, 2016

Conversation

rphmeier
Copy link
Contributor

Part of #1803

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 21, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e21aef4 on no-ethcore-common into * on master*.

@gavofyork
Copy link
Contributor

yey. a PR which add 45 lines of code and results in no bugs being fixed, case being tested nor feature being implemented.

@rphmeier
Copy link
Contributor Author

rphmeier commented Oct 24, 2016

@gavofyork From the wiki: "readable code == verifiable code". Just trying to keep the "our codebase is lovely" statement true.

Glob imports are bad for readability/flexibility, but sometimes acceptable. I find that glob exports are very rarely so.

@gavofyork
Copy link
Contributor

gavofyork commented Oct 24, 2016

perhaps we should being listing which language components we plan to use in the preamble of each and every file.

@use_language_component (u8, u32, usize, str, &, &mut, for-loop, if-statement, match-statement, function, enum, struct);

because, as every great C programmer knows, only with a sufficiently large amount of boilerplate at the top of every file can the codebase truly be considered to be "lovely".

@rphmeier
Copy link
Contributor Author

Part of the reason is to encourage new contributors. Once you've attained a certain degree of familiarity with the codebase, you start to get a feel for where things are coming from (although that doesn't really excuse the re-exports of standard library types from our own modules), but I remember the overwhelming feeling when I first started working on parity was less of a "what does this code do?" than a "where the hell does this type come from?"

Part of it is to allow the reader to immediately draw connections of what dependencies in the rest of the codebase the piece of code they're looking at presently has. Classifying explicit imports as boilerplate is a bit dismissive of the entire feature of namespaces IMO. The goal is to immediately enable the reader to identify where a type is declared so they can inspect its implementation as well, and to avoid conflicts when writing code. The category of util::* is still far too wide, since it encompasses not only our own 10 or 20 thousand LoC, but also a grab bag of various crates and modules from std. Not particularly searchable if you ask me.

@gavofyork
Copy link
Contributor

gavofyork commented Oct 24, 2016

I care about as much that 90% of files in the codebase are dependent on U256 and H256 as I do that 90% of the files each make use of u64 and String. We don't require u64 or str to be imported individually (a good decision on the part of Rust devs in my mind) and so I don't see why the codebase needs to be peppered with low-value lines like use uint::U256;.

boilerplate is a bit dismissive

"boilerplate" is defined as "repeated code of little value and substantial footprint". I'd say that covers the incessant use uint::U256; in the codebase of a project where 256-bit arithmetic is fundamental in the design.

@rphmeier
Copy link
Contributor Author

rphmeier commented Oct 24, 2016

Agreed. The standard way of handling this is to use a prelude module as std does (use std::prelude::v1::* is inserted into every file). We have the bigint (note: not part of util directly) crate which exposes all the bigint and hash types. It ought to have a prelude so we can use bigint::prelude::*. You can see this kind of design in std::io, rayon, and probably several others. Note that this differs severely from stuffing every type ever witnessed by the developers into a single prelude, by limiting the re-exports categorically and naming the preludes themselves descriptively.

@rphmeier
Copy link
Contributor Author

I've added a prelude module to ethcore-bigint -- now we can use bigint::prelude::* and get all the types and trait implementations defined there, cleanly and explicitly. I'd favor using this crate directly rather than the re-exports through util.

@gavofyork gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 24, 2016
@gavofyork gavofyork merged commit 0fedc27 into master Oct 24, 2016
@gavofyork gavofyork deleted the no-ethcore-common branch October 24, 2016 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants