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

[dependencies]: remove util/macros #11501

Merged
merged 5 commits into from
Feb 19, 2020
Merged

[dependencies]: remove util/macros #11501

merged 5 commits into from
Feb 19, 2020

Conversation

niklasad1
Copy link
Collaborator

@niklasad1 niklasad1 commented Feb 18, 2020

Closing #11478

Nothing very exciting in this PR to summarize:

  • Remove util/macros crate, move flush! and flushln! to ethcore/json_tests.
  • Rename: flush, flushln -> flushed_write, flushed_writeln (those were misleading)
  • Replace macros with maplit
  • Some changes because slice_into and vec_into are removed

Note, I didn't replace flush, flushln with logging in the json-tests. Let's keep out of this PR

// TODO(niklasad1): use expect or unwrap only used for tests?!
#[doc(hidden)]
pub fn write_and_flush(s: String) {
let _ = std::io::Write::write_all(&mut std::io::stdout(), s.as_bytes());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed this to write_all instead of write seems better because we are not notified if only a part of the buffer is written, thoughts?

@@ -2,12 +2,14 @@
name = "migration-rocksdb"
version = "0.1.0"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
Copy link
Collaborator Author

@niklasad1 niklasad1 Feb 18, 2020

Choose a reason for hiding this comment

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

sorry I couldn't resist, it was such a simple fix.

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, good riddance.

@dvdplm
Copy link
Collaborator

dvdplm commented Feb 18, 2020

(what's up with the force-push, makes it hard to review additions)

@@ -37,8 +37,7 @@ pub fn json_difficulty_test<H: FnMut(&str, HookType)>(
for (name, test) in tests.into_iter() {
start_stop_hook(&name, HookType::OnStart);

flush!(" - {}...", name);
println!(" - {}...", name);
Copy link
Collaborator Author

@niklasad1 niklasad1 Feb 18, 2020

Choose a reason for hiding this comment

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

redundant, let's replace with writeln + flush.

@niklasad1
Copy link
Collaborator Author

(what's up with the force-push, makes it hard to review additions)

Sorry, my bad it just think is so ugly will fix missing file but will merge from now on. Didn't think you would review it so fast.

} else {
"".into()
};
let empty_steps = header_empty_steps(header).as_ref().map_or(String::new(), |empty_steps| {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this change is also unrelated, I found this piece very hard to read especially the fold

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Looks good

ethcore/src/json_tests/macros.rs Outdated Show resolved Hide resolved
util/migration-rocksdb/src/lib.rs Outdated Show resolved Hide resolved
H512::from_low_u64_be(10) => 50
],
propagated_to: {
let mut map = BTreeMap::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not a btreemap!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nit, I think I started to investigate if we could get rid of map completely but forgot this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought you left it on purpose. Why use a macro when it doesn’t save much typing and doesn’t make the code clearer?

Copy link
Collaborator Author

@niklasad1 niklasad1 Feb 19, 2020

Choose a reason for hiding this comment

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

I was just too lazy, my rule of thumb is when we have more than 2-3 items in a BTreeMap then I use btreemap!.

At least after this change we are consistent in the crate but I saw that we can easily remove maplit in the rpc crate

EDIT: when it comes to dev-dependencies I'm more willing to use those macros than in production code

@ordian ordian merged commit a49950e into master Feb 19, 2020
@ordian ordian deleted the na-kill-util_macros branch February 19, 2020 12:07
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. M5-dependencies 🖇 Dependencies. labels Feb 21, 2020
ordian added a commit that referenced this pull request Mar 6, 2020
* master: (27 commits)
  Faster kill_garbage (#11514)
  [EngineSigner]: don't sign message with only zeroes (#11524)
  fix compilation warnings (#11522)
  [ethcore cleanup]: various unrelated fixes from `#11493` (#11507)
  Add benchmark for transaction execution (#11509)
  Add Smart Contract License v1.0
  Misc fixes (#11510)
  [dependencies]: unify `rustc-hex` (#11506)
  Activate on-chain randomness in POA Sokol (#11505)
  Grab bag of cleanup (#11504)
  Implement eth/64 (EIP-2364) and drop support for eth/62 (#11472)
  [dependencies]: remove `util/macros` (#11501)
  OpenEthereum bootnodes are added (#11499)
  [ci benches]: use `all-features` (#11496)
  [verification]: make test-build compile standalone (#11495)
  complete null-signatures removal (#11491)
  Include the seal when populating the header for a new block (#11475)
  fix compilation warnings (#11492)
  cargo update -p cmake (#11490)
  update to published rlp-derive (#11489)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M5-dependencies 🖇 Dependencies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants