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

Conversation

@andresilva
Copy link
Contributor

@andresilva andresilva commented Jul 7, 2018

Currently the coverage build fails because futures-timer fails to compile with -C link-dead-code (https://gitlab.parity.io/parity/parity/-/jobs/91245). This issue has been reported to futures-timer (async-rs/futures-timer#2) but has remained unsolved for months. It should be fixed by rustc eventually (rust-lang/rust#45629).

@andresilva andresilva added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 7, 2018
@5chdn 5chdn added this to the 2.0 milestone Jul 7, 2018
@andresilva andresilva force-pushed the andre/remove-futures-timer branch from ee8a082 to 977d045 Compare July 7, 2018 14:17
pub struct Client {
core: mpsc::Sender<ChanItem>,
refs: Arc<AtomicUsize>,
timer: Timer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/spaces/tab

Client {
core: self.core.clone(),
refs: self.refs.clone(),
timer: self.timer.clone(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/spaces/tab

Ok(Client {
core: tx_proto,
refs: Arc::new(AtomicUsize::new(1)),
timer: Timer::default(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/spaces/tab

Box::new(future)
.and_then(future::result);

Box::new(self.timer.timeout(future, maxdur))
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/spaces/tab

/// Too many redirects have been encountered.
TooManyRedirects,
/// tokio-timer gave us an error.
Timer(tokio_timer::TimerError),
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/spaces/tab

tokio_timer::TimeoutError::TimedOut(_) => Error::Timeout,
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/spaces/tab

Copy link
Collaborator

@niklasad1 niklasad1 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 just replace the spaces with tabs!

let d = Duration::from_secs(req.uri().query().unwrap_or("0").parse().unwrap());
Box::new(Delay::new(d).from_err().map(|_| Response::new()))
Box::new(self.0.sleep(d)
.map_err(|_| return io::Error::new(io::ErrorKind::Other, "timer error"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/spaces/tab

@andresilva
Copy link
Contributor Author

Yeah I had editorconfig disabled because I was having some issues with magit. I will reformat everything and squash.

@niklasad1 niklasad1 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 Jul 7, 2018
Currently the coverage build fails because `futures-timer` fails to compile with
`-C link-dead-code`. This issue has been reported to `futures-timer`
(async-rs/futures-timer#2) but has remained unsolved
for months. It should be fixed by rustc eventually
(rust-lang/rust#45629).
@andresilva andresilva force-pushed the andre/remove-futures-timer branch from 977d045 to 6c25589 Compare July 7, 2018 16:09
@andresilva andresilva added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jul 8, 2018
@andresilva andresilva force-pushed the andre/remove-futures-timer branch from 3850a34 to b464bd1 Compare July 8, 2018 16:54
@andresilva
Copy link
Contributor Author

andresilva added a commit that referenced this pull request Jul 8, 2018
@5chdn
Copy link
Contributor

5chdn commented Jul 8, 2018

Amazing!

@andresilva
Copy link
Contributor Author

Better coverage report that doesn't include the target folder: https://codecov.io/gh/paritytech/parity/tree/71ee31d1052f2cd6e74fdc28e5eb5edf53e3d282/parity

@andresilva andresilva force-pushed the andre/remove-futures-timer branch from 71ee31d to f335830 Compare July 8, 2018 21:40
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

👍

@ascjones ascjones merged commit ca6edca into master Jul 9, 2018
@ascjones ascjones deleted the andre/remove-futures-timer branch July 9, 2018 08:59
dvdplm added a commit that referenced this pull request Jul 9, 2018
* master:
  fetch: replace futures-timer with tokio-timer (#9066)
ordian added a commit to ordian/parity that referenced this pull request Jul 9, 2018
…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity:
  docs: add changelog for 1.10.9 stable and 1.11.6 beta (openethereum#9069)
  Enable test in `miner/pool/test` (openethereum#9072)
  fetch: replace futures-timer with tokio-timer (openethereum#9066)
  remove util-error (openethereum#9054)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants