Closed alphastrata closed 5 years ago
Hey, thanks for the pull request. It will probably be a while before I can review it properly.
As a quick side note, a new pull request typically means a new branch. In the future, I would recommend that you close a branch after it's been merged and start a new branch from the latest master branch, rather than continuing to commit on the merged branch.
Hey, thanks for the pull request. It will probably be a while before I can review it properly.
As a quick side note, a new pull request typically means a new branch. In the future, I would recommend that you close a branch after it's been merged and start a new branch from the latest master branch, rather than continuing to commit on the merged branch.
I will do that!
After a clean install of rust my rustfmt
is working now (looks as though I was just using cargo fmt
instead of cargo +nightly fmt
as I'd had nightly set as my default rust & forgotten about it.)
Overall, this looks pretty good. One comment though - you have -> Result<(), Box<dyn Error>>{
in a few places in the examples. There should be a space before the {
. There's also one case where you need a space after the ->
.
Aside from that, just fix up the rustfmt warnings (rustfmt won't catch style issues in the doctests, unfortunately, have to do that manually) and this should be good to go.
Overall, this looks pretty good. One comment though - you have
-> Result<(), Box<dyn Error>>{
in a few places in the examples. There should be a space before the{
. There's also one case where you need a space after the->
.Aside from that, just fix up the rustfmt warnings (rustfmt won't catch style issues in the doctests, unfortunately, have to do that manually) and this should be good to go.
Cool, I am pretty sure this can be done over the upcoming weekend. One question, once above changes are made does the pull request need to be resubmitted or will it automatically be sent to Travis and so on??
Just push more commits, Travis-CI will run again automatically.
Looks good to me. Thanks!
Got them all, (bar one or two)
I've tentatively removed
unwrap()
s from the#[test]
s as well, not sure if that's how things are usually done; The commits are split up just in case you want to cherry pick them on a per-file basis if required, I think I've split them up & labelled them clearly enough.I'm concerned about the 'unused_must_use' warnings that src/device.rs is now throwing since removing the
.unwrap()
from the firstinit
test -- I don't know what would normally be done here...There should be fewer issues with the formatting this time around.