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

use constant durations #8278

Merged
merged 4 commits into from
Apr 2, 2018
Merged

use constant durations #8278

merged 4 commits into from
Apr 2, 2018

Conversation

rleungx
Copy link
Contributor

@rleungx rleungx commented Mar 30, 2018

Closes #8260.

@parity-cla-bot
Copy link

It looks like @rleungx signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

tomusdrw
tomusdrw approved these changes Mar 30, 2018
View reviewed changes
secret_store/src/key_server_cluster/cluster.rs Outdated
@@ -1388,11 +1388,11 @@ pub mod tests {
let mut core = Core::new().unwrap();
let clusters = make_clusters(&core, 6028, 3);
run_clusters(&clusters);
loop_until(&mut core, time::Duration::from_millis(300), || clusters.iter().all(all_connections_established));
loop_until(&mut core, Duration::from_millis(300), || clusters.iter().all(all_connections_established));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps those Durations could be moved to constants as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any naming suggestion? Something like TIMEOUT?

@tomusdrw tomusdrw added M4-core ⛓ Core client code / Rust. A4-awaitingci 🤖 Pull request is waiting for changes on the CI to complete tests before review/merge can begin. A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A4-awaitingci 🤖 Pull request is waiting for changes on the CI to complete tests before review/merge can begin. labels Mar 30, 2018
@tomaka
Copy link
Contributor

tomaka commented Mar 31, 2018

In my opinion the constants should be renamed as well, for example TIMEOUT_SECS renamed to TIMEOUT, UPDATE_INTERVAL_MS to UPDATE_INTERVAL, and so on.

tomaka
tomaka suggested changes Mar 31, 2018
View reviewed changes
Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Constants should be renamed as well to remove the unit.

@tomusdrw tomusdrw merged commit 9c9ddac into openethereum:master Apr 2, 2018
@5chdn 5chdn added this to the 1.11 milestone Apr 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tomaka tomaka tomaka approved these changes

@tomusdrw tomusdrw tomusdrw approved these changes

Assignees
No one assigned
Labels
A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
1.11 Prosperity
Development

Successfully merging this pull request may close these issues.
5 participants
@rleungx @parity-cla-bot @tomaka @tomusdrw @5chdn