Merge #57

57: Add clippy r=matthiasbeyer a=matthiasbeyer

This PR adds `cargo-clippy` and cleans up all warnings.

Co-authored-by: Matthias Beyer <[email protected]>

Author: bors[bot]

.github/workflows/ci.yml

@@ -71,6 +71,21 @@ jobs:
           args: -- --check
 
 
+  clippy:
+    name: clippy
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions-rs/toolchain@v1
+        with:
+          toolchain: 1.60.0
+          override: true
+      - uses: swatinem/rust-cache@v1
+      - run: rustup component add clippy
+      - name: cargo-clippy
+        run: cargo clippy --all --all-targets --all-features
+
+
   # We need some "accummulation" job here because bors fails (timeouts) to
   # listen on matrix builds.
   # Hence, we have some kind of dummy here that bors can listen on
@@ -81,6 +96,7 @@ jobs:
       - check
       - deny
       - fmt
+      - clippy
     runs-on: ubuntu-latest
     steps:
       - name: CI succeeded

src/lib.rs

@@ -107,7 +107,7 @@ pub mod errors {
                 description("The process didn't end within the given timeout")
                 display("Timeout Error: Expected {} but got \"{}\" (after waiting {} ms)",
                         expected, got, (timeout.as_secs() * 1000) as u32
-                        + timeout.subsec_nanos() / 1_000_000)
+                        + timeout.subsec_millis())
             }
             EmptyProgramName {
                 description("The provided program name is empty.")

src/process.rs

@@ -123,7 +123,7 @@ impl PtyProcess {
                 }
                 ForkResult::Parent { child: child_pid } => Ok(PtyProcess {
                     pty: master_fd,
-                    child_pid: child_pid,
+                    child_pid,
                     kill_timeout: None,
                 }),
             }
@@ -142,7 +142,7 @@ impl PtyProcess {
     /// the process does not react to a normal kill. If kill_timeout is set the process is
     /// `kill -9`ed after duration
     pub fn set_kill_timeout(&mut self, timeout_ms: Option<u64>) {
-        self.kill_timeout = timeout_ms.and_then(|millis| Some(time::Duration::from_millis(millis)));
+        self.kill_timeout = timeout_ms.map(time::Duration::from_millis);
     }
 
     /// Get status of child process, non-blocking.
@@ -228,11 +228,8 @@ impl PtyProcess {
 
 impl Drop for PtyProcess {
     fn drop(&mut self) {
-        match self.status() {
-            Some(wait::WaitStatus::StillAlive) => {
-                self.exit().expect("cannot exit");
-            }
-            _ => {}
+        if let Some(wait::WaitStatus::StillAlive) = self.status() {
+            self.exit().expect("cannot exit");
         }
     }
 }
@@ -253,7 +250,7 @@ mod tests {
             let f = process.get_file_handle();
             let mut writer = LineWriter::new(&f);
             let mut reader = BufReader::new(&f);
-            writer.write(b"hello cat\n")?;
+            let _ = writer.write(b"hello cat\n")?;
             let mut buf = String::new();
             reader.read_line(&mut buf)?;
             assert_eq!(buf, "hello cat\r\n");

src/reader.rs

@@ -14,6 +14,7 @@ enum PipeError {
 }
 
 #[derive(Debug)]
+#[allow(clippy::upper_case_acronyms)]
 enum PipedChar {
     Char(u8),
     EOF,
@@ -30,13 +31,13 @@ pub enum ReadUntil {
 impl fmt::Display for ReadUntil {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         let printable = match self {
-            &ReadUntil::String(ref s) if s == "\n" => "\\n (newline)".to_string(),
-            &ReadUntil::String(ref s) if s == "\r" => "\\r (carriage return)".to_string(),
-            &ReadUntil::String(ref s) => format!("\"{}\"", s),
-            &ReadUntil::Regex(ref r) => format!("Regex: \"{}\"", r),
-            &ReadUntil::EOF => "EOF (End of File)".to_string(),
-            &ReadUntil::NBytes(n) => format!("reading {} bytes", n),
-            &ReadUntil::Any(ref v) => {
+            ReadUntil::String(ref s) if s == "\n" => "\\n (newline)".to_string(),
+            ReadUntil::String(ref s) if s == "\r" => "\\r (carriage return)".to_string(),
+            ReadUntil::String(ref s) => format!("\"{}\"", s),
+            ReadUntil::Regex(ref r) => format!("Regex: \"{}\"", r),
+            ReadUntil::EOF => "EOF (End of File)".to_string(),
+            ReadUntil::NBytes(n) => format!("reading {} bytes", n),
+            ReadUntil::Any(ref v) => {
                 let mut res = Vec::new();
                 for r in v {
                     res.push(r.to_string());
@@ -62,35 +63,29 @@ impl fmt::Display for ReadUntil {
 /// 2. position after match
 pub fn find(needle: &ReadUntil, buffer: &str, eof: bool) -> Option<(usize, usize)> {
     match needle {
-        &ReadUntil::String(ref s) => buffer.find(s).and_then(|pos| Some((pos, pos + s.len()))),
-        &ReadUntil::Regex(ref pattern) => {
-            if let Some(mat) = pattern.find(buffer) {
-                Some((mat.start(), mat.end()))
-            } else {
-                None
-            }
-        }
-        &ReadUntil::EOF => {
+        ReadUntil::String(ref s) => buffer.find(s).map(|pos| (pos, pos + s.len())),
+        ReadUntil::Regex(ref pattern) => pattern.find(buffer).map(|mat| (mat.start(), mat.end())),
+        ReadUntil::EOF => {
             if eof {
                 Some((0, buffer.len()))
             } else {
                 None
             }
         }
-        &ReadUntil::NBytes(n) => {
-            if n <= buffer.len() {
-                Some((0, n))
-            } else if eof && buffer.len() > 0 {
+        ReadUntil::NBytes(n) => {
+            if *n <= buffer.len() {
+                Some((0, *n))
+            } else if eof && buffer.is_empty() {
                 // reached almost end of buffer, return string, even though it will be
                 // smaller than the wished n bytes
                 Some((0, buffer.len()))
             } else {
                 None
             }
         }
-        &ReadUntil::Any(ref any) => {
+        ReadUntil::Any(ref any) => {
             for read_until in any {
-                if let Some(pos_tuple) = find(&read_until, buffer, eof) {
+                if let Some(pos_tuple) = find(read_until, buffer, eof) {
                     return Some(pos_tuple);
                 }
             }
@@ -131,7 +126,7 @@ impl NBReader {
                 loop {
                     match reader.read(&mut byte) {
                         Ok(0) => {
-                            let _ = tx.send(Ok(PipedChar::EOF)).chain_err(|| "cannot send")?;
+                            tx.send(Ok(PipedChar::EOF)).chain_err(|| "cannot send")?;
                             break;
                         }
                         Ok(_) => {
@@ -155,7 +150,7 @@ impl NBReader {
             reader: rx,
             buffer: String::with_capacity(1024),
             eof: false,
-            timeout: timeout.and_then(|millis| Some(time::Duration::from_millis(millis))),
+            timeout: timeout.map(time::Duration::from_millis),
         }
     }
 
@@ -252,8 +247,8 @@ impl NBReader {
                         needle.to_string(),
                         self.buffer
                             .clone()
-                            .replace("\n", "`\\n`\n")
-                            .replace("\r", "`\\r`")
+                            .replace('\n', "`\\n`\n")
+                            .replace('\r', "`\\r`")
                             .replace('\u{1b}', "`^`"),
                         timeout,
                     )
@@ -270,7 +265,7 @@ impl NBReader {
     pub fn try_read(&mut self) -> Option<char> {
         // discard eventual errors, EOF will be handled in read_until correctly
         let _ = self.read_into_buffer();
-        if self.buffer.len() > 0 {
+        if self.buffer.is_empty() {
             self.buffer.drain(..1).last()
         } else {
             None
@@ -293,9 +288,9 @@ mod tests {
         );
         // check for EOF
         match r.read_until(&ReadUntil::NBytes(10)) {
-            Ok(_) => assert!(false),
+            Ok(_) => panic!(),
             Err(Error(ErrorKind::EOF(_, _, _), _)) => {}
-            Err(Error(_, _)) => assert!(false),
+            Err(Error(_, _)) => panic!(),
         }
     }
 

src/session.rs

@@ -32,7 +32,7 @@ impl<W: Write> StreamSession<W> {
         let mut len = self.send(line)?;
         len += self
             .writer
-            .write(&['\n' as u8])
+            .write(&[b'\n'])
             .chain_err(|| "cannot write newline")?;
         Ok(len)
     }
@@ -53,8 +53,8 @@ impl<W: Write> StreamSession<W> {
     /// E.g. `send_control('c')` sends ctrl-c. Upper/smaller case does not matter.
     pub fn send_control(&mut self, c: char) -> Result<()> {
         let code = match c {
-            'a'..='z' => c as u8 + 1 - 'a' as u8,
-            'A'..='Z' => c as u8 + 1 - 'A' as u8,
+            'a'..='z' => c as u8 + 1 - b'a',
+            'A'..='Z' => c as u8 + 1 - b'A',
             '[' => 27,
             '\\' => 28,
             ']' => 29,
@@ -105,7 +105,7 @@ impl<W: Write> StreamSession<W> {
     /// Wait until we see EOF (i.e. child process has terminated)
     /// Return all the yet unread output
     pub fn exp_eof(&mut self) -> Result<String> {
-        self.exp(&ReadUntil::EOF).and_then(|(_, s)| Ok(s))
+        self.exp(&ReadUntil::EOF).map(|(_, s)| s)
     }
 
     /// Wait until provided regex is seen on stdout of child process.
@@ -116,26 +116,23 @@ impl<W: Write> StreamSession<W> {
     /// Note that `exp_regex("^foo")` matches the start of the yet consumed output.
     /// For matching the start of the line use `exp_regex("\nfoo")`
     pub fn exp_regex(&mut self, regex: &str) -> Result<(String, String)> {
-        let res = self
-            .exp(&ReadUntil::Regex(
-                Regex::new(regex).chain_err(|| "invalid regex")?,
-            ))
-            .and_then(|s| Ok(s));
-        res
+        self.exp(&ReadUntil::Regex(
+            Regex::new(regex).chain_err(|| "invalid regex")?,
+        ))
     }
 
     /// Wait until provided string is seen on stdout of child process.
     /// Return the yet unread output (without the matched string)
     pub fn exp_string(&mut self, needle: &str) -> Result<String> {
         self.exp(&ReadUntil::String(needle.to_string()))
-            .and_then(|(s, _)| Ok(s))
+            .map(|(s, _)| s)
     }
 
     /// Wait until provided char is seen on stdout of child process.
     /// Return the yet unread output (without the matched char)
     pub fn exp_char(&mut self, needle: char) -> Result<String> {
         self.exp(&ReadUntil::String(needle.to_string()))
-            .and_then(|(s, _)| Ok(s))
+            .map(|(s, _)| s)
     }
 
     /// Wait until any of the provided needles is found.
@@ -362,7 +359,7 @@ impl Drop for PtyReplSession {
     fn drop(&mut self) {
         if let Some(ref cmd) = self.quit_command {
             self.pty_session
-                .send_line(&cmd)
+                .send_line(cmd)
                 .expect("could not run `exit` on bash process");
         }
     }
@@ -399,7 +396,7 @@ pub fn spawn_bash(timeout: Option<u64>) -> Result<PtyReplSession> {
     // would set as PS1 and we cannot know when is the right time
     // to set the new PS1
     let mut rcfile = tempfile::NamedTempFile::new().unwrap();
-    rcfile
+    let _ = rcfile
         .write(
             b"include () { [[ -f \"$1\" ]] && source \"$1\"; }\n\
                   include /etc/bash.bashrc\n\
@@ -411,10 +408,7 @@ pub fn spawn_bash(timeout: Option<u64>) -> Result<PtyReplSession> {
     let mut c = Command::new("bash");
     c.args(&[
         "--rcfile",
-        rcfile
-            .path()
-            .to_str()
-            .unwrap_or_else(|| return "temp file does not exist".into()),
+        rcfile.path().to_str().unwrap_or("temp file does not exist"),
     ]);
     spawn_command(c, timeout).and_then(|p| {
         let new_prompt = "[REXPECT_PROMPT>";
@@ -439,13 +433,11 @@ pub fn spawn_bash(timeout: Option<u64>) -> Result<PtyReplSession> {
 ///
 /// This is just a proof of concept implementation (and serves for documentation purposes)
 pub fn spawn_python(timeout: Option<u64>) -> Result<PtyReplSession> {
-    spawn_command(Command::new("python"), timeout).and_then(|p| {
-        Ok(PtyReplSession {
-            prompt: ">>> ".to_string(),
-            pty_session: p,
-            quit_command: Some("exit()".to_string()),
-            echo_on: true,
-        })
+    spawn_command(Command::new("python"), timeout).map(|p| PtyReplSession {
+        prompt: ">>> ".to_string(),
+        pty_session: p,
+        quit_command: Some("exit()".to_string()),
+        echo_on: true,
     })
 }
 
@@ -484,9 +476,9 @@ mod tests {
         || -> Result<()> {
             let mut p = spawn("sleep 3", Some(1000)).expect("cannot run sleep 3");
             match p.exp_eof() {
-                Ok(_) => assert!(false, "should raise Timeout"),
+                Ok(_) => panic!("should raise Timeout"),
                 Err(Error(ErrorKind::Timeout(_, _, _), _)) => {}
-                Err(_) => assert!(false, "should raise TimeOut"),
+                Err(_) => panic!("should raise TimeOut"),
             }
             Ok(())
         }()
@@ -533,7 +525,7 @@ mod tests {
                 ReadUntil::String("Hi".to_string()),
             ]) {
                 Ok(s) => assert_eq!(("".to_string(), "Hi\r".to_string()), s),
-                Err(e) => assert!(false, format!("got error: {}", e)),
+                Err(e) => panic!("got error: {}", e),
             }
             Ok(())
         }()
@@ -544,9 +536,9 @@ mod tests {
     fn test_expect_empty_command_error() {
         let p = spawn("", Some(1000));
         match p {
-            Ok(_) => assert!(false, "should raise an error"),
+            Ok(_) => panic!("should raise an error"),
             Err(Error(ErrorKind::EmptyProgramName, _)) => {}
-            Err(_) => assert!(false, "should raise EmptyProgramName"),
+            Err(_) => panic!("should raise EmptyProgramName"),
         }
     }