Merge pull request #126 from Goval-Community/refactor

More refactoring and lints

Cargo.toml

@@ -32,7 +32,6 @@ tracing-futures = "0.2.5"
 tracing-subscriber = { version = "0.3.18", features = ["tracing-log"]}
 futures-channel = "0.3.26"
 futures-util = "0.3.26"
-log = { version = "0.4.17", features = ["kv_unstable", "kv_unstable_serde"] }
 prost = "0.12.4"
 prost-types = "0.12.3"
 serde_json = "1.0.115"

services/src/chat.rs

@@ -10,7 +10,7 @@ use async_trait::async_trait;
 use tracing::warn;
 
 impl Chat {
-    pub fn new() -> Chat {
+    pub const fn new() -> Chat {
         Chat { history: vec![] }
     }
 }
@@ -30,14 +30,12 @@ impl traits::Service for Chat {
 
         match body {
             goval::command::Body::ChatMessage(msg) => {
-                info.send(message, SendSessions::EveryoneExcept(session))
-                    .await?;
+                info.send(message, SendSessions::EveryoneExcept(session))?;
                 self.history.push(msg);
                 Ok(None)
             }
             goval::command::Body::ChatTyping(_) => {
-                info.send(message, SendSessions::EveryoneExcept(session))
-                    .await?;
+                info.send(message, SendSessions::EveryoneExcept(session))?;
                 Ok(None)
             }
             _ => {

services/src/dotreplit.rs

@@ -22,7 +22,7 @@ impl traits::Service for DotReplit {
         match body {
             goval::command::Body::DotReplitGetRequest(_) => {
                 let mut dotreplit = goval::Command::default();
-                let inner: goval::DotReplit = _info.dotreplit.read().await.clone().into();
+                let inner: goval::DotReplit = (&*_info.dotreplit.read().await).into();
 
                 dotreplit.body = Some(goval::command::Body::DotReplitGetResponse(
                     goval::DotReplitGetResponse {

services/src/exec.rs

@@ -32,8 +32,7 @@ impl traits::Service for Exec {
                             ..Default::default()
                         },
                         crate::SendSessions::Everyone,
-                    )
-                    .await?;
+                    )?;
                     return Ok(None);
                 }
 
@@ -45,19 +44,17 @@ impl traits::Service for Exec {
                         ..Default::default()
                     },
                     crate::SendSessions::Everyone,
-                )
-                .await?;
+                )?;
                 self.running = true;
                 self.current_ref = message.r#ref;
-                Proc::new(exec.args, info.id, info.sender.clone(), Some(exec.env)).await?;
+                Proc::new(exec.args, info.id, info.sender.clone(), Some(&exec.env))?;
                 info.send(
                     goval::Command {
                         body: Some(goval::command::Body::State(goval::State::Running.into())),
                         ..Default::default()
                     },
                     crate::SendSessions::Everyone,
-                )
-                .await?;
+                )?;
             }
         }
 
@@ -78,8 +75,7 @@ impl traits::Service for Exec {
                     ..Default::default()
                 },
                 crate::SendSessions::Everyone,
-            )
-            .await?;
+            )?;
         } else {
             info.send(
                 goval::Command {
@@ -90,8 +86,7 @@ impl traits::Service for Exec {
                     ..Default::default()
                 },
                 crate::SendSessions::Everyone,
-            )
-            .await?;
+            )?;
         }
 
         self.current_ref = String::new();
@@ -102,22 +97,20 @@ impl traits::Service for Exec {
                 ..Default::default()
             },
             crate::SendSessions::Everyone,
-        )
-        .await?;
+        )?;
 
         if !self.queue.is_empty() {
             self.running = true;
             let item = self.queue.swap_remove(0);
-            Proc::new(item.0.args, info.id, info.sender.clone(), Some(item.0.env)).await?;
+            Proc::new(item.0.args, info.id, info.sender.clone(), Some(&item.0.env))?;
             self.current_ref = item.1;
             info.send(
                 goval::Command {
                     body: Some(goval::command::Body::State(goval::State::Running.into())),
                     ..Default::default()
                 },
                 crate::SendSessions::Everyone,
-            )
-            .await?;
+            )?;
         }
 
         Ok(())

services/src/fsevents.rs

@@ -37,23 +37,20 @@ impl traits::Service for FSEvents {
             Some(body) => body,
         };
 
-        match body {
-            goval::command::Body::SubscribeFile(subscribe) => {
-                let mut files = vec![];
-                for file in subscribe.files {
-                    files.push(file.path);
-                }
-
-                self.watcher.watch(files).await?;
-                Ok(Some(goval::Command {
-                    body: Some(goval::command::Body::Ok(goval::Ok {})),
-                    ..Default::default()
-                }))
-            }
-            _ => {
-                warn!(cmd = ?message, "Unknown fs event command");
-                Ok(None)
+        if let goval::command::Body::SubscribeFile(subscribe) = body {
+            let mut files = vec![];
+            for file in subscribe.files {
+                files.push(file.path);
             }
+
+            self.watcher.watch(files)?;
+            Ok(Some(goval::Command {
+                body: Some(goval::command::Body::Ok(goval::Ok {})),
+                ..Default::default()
+            }))
+        } else {
+            warn!(cmd = ?message, "Unknown fs event command");
+            Ok(None)
         }
     }
 
@@ -114,13 +111,12 @@ impl traits::Service for FSEvents {
                 ..Default::default()
             },
             crate::SendSessions::Everyone,
-        )
-        .await?;
+        )?;
         Ok(())
     }
 
     async fn shutdown(self: Box<FSEvents>, _info: &super::types::ChannelInfo) -> Result<()> {
-        self.watcher.shutdown().await;
+        self.watcher.shutdown();
         Ok(())
     }
 }

services/src/gcsfiles.rs

@@ -46,10 +46,10 @@ impl traits::Service for GCSFiles {
                     }
                 }
 
-                let mut ret = goval::Command::default();
+                let mut ret_cmd = goval::Command::default();
                 let _inner = goval::Files { files: res };
-                ret.body = Some(goval::command::Body::Files(_inner));
-                Ok(Some(ret))
+                ret_cmd.body = Some(goval::command::Body::Files(_inner));
+                Ok(Some(ret_cmd))
             }
             goval::command::Body::Mkdir(dir) => {
                 fs::create_dir_all(dir.path).await?;
@@ -107,9 +107,9 @@ impl traits::Service for GCSFiles {
             goval::command::Body::Remove(file) => {
                 let stat = fs::metadata(&file.path).await?;
                 if stat.is_dir() {
-                    fs::remove_dir_all(&file.path).await?
+                    fs::remove_dir_all(&file.path).await?;
                 } else {
-                    fs::remove_file(&file.path).await?
+                    fs::remove_file(&file.path).await?;
                 }
 
                 let ret = goval::Command {

services/src/git.rs

@@ -82,8 +82,7 @@ impl traits::Service for Git {
                     )),
                     ..Default::default()
                 };
-                info.send(token_req, crate::SendSessions::Only(session))
-                    .await?;
+                info.send(token_req, crate::SendSessions::Only(session))?;
 
                 self.replspace.insert(nonce, respond);
             }
@@ -98,8 +97,7 @@ impl traits::Service for Git {
                     )),
                     ..Default::default()
                 };
-                info.send(token_req, crate::SendSessions::Only(session))
-                    .await?;
+                info.send(token_req, crate::SendSessions::Only(session))?;
 
                 self.replspace.insert(nonce, respond);
             }

services/src/lib.rs

@@ -1,3 +1,25 @@
+#![feature(extract_if)]
+#![warn(
+    clippy::pedantic,
+    clippy::unwrap_used,
+    clippy::redundant_clone,
+    clippy::missing_const_for_fn
+)]
+#![allow(
+    clippy::module_name_repetitions,
+    clippy::must_use_candidate,
+    clippy::return_self_not_must_use,
+
+    // TODO: investigate performance impact of i__::try_from
+    clippy::cast_possible_truncation,
+    clippy::cast_possible_wrap,
+
+    // TODO: remove and fix these 
+    clippy::missing_errors_doc,
+    clippy::missing_panics_doc,
+    clippy::too_many_lines
+)]
+
 mod chat;
 mod dotreplit;
 mod exec;
@@ -24,7 +46,7 @@ pub use types::*;
 
 pub struct Channel {
     info: ChannelInfo,
-    _inner: Box<dyn traits::Service + Send>,
+    inner: Box<dyn traits::Service + Send>,
 }
 
 // Public functions
@@ -54,27 +76,25 @@ impl Channel {
             "presence" => Box::new(presence::Presence::new()),
             "ot" => Box::new(ot::OT::new(sender).await?),
             "snapshot" => Box::new(snapshot::Snapshot {}),
-            "output" => Box::new(output::Output::new().await),
-            "shell" => Box::new(shell::Shell::new(&info).await?),
+            "output" => Box::new(output::Output::new()),
+            "shell" => Box::new(shell::Shell::new(&info)?),
             "toolchain" => Box::new(toolchain::Toolchain {}),
             "git" => Box::new(git::Git::new()),
             "exec" => Box::new(exec::Exec::new()),
             "dotreplit" => Box::new(dotreplit::DotReplit {}),
             "fsevents" => Box::new(fsevents::FSEvents::new(sender).await?),
-            "audio" => Box::new(stub::Stub {}), // Will never be supported
-            "null" => Box::new(stub::Stub {}),  // This channel never does anything
-            "open" => Box::new(stub::Stub {}),  // Stub until infra is set up to handle this
+            "audio" | "null" => Box::new(stub::Stub {}), // Audio will never be supported and null does nothing
             _ => return Err(format_err!("Unknown service: {}", service)),
         };
 
         Ok(Channel {
             info,
-            _inner: channel,
+            inner: channel,
         })
     }
 
     pub async fn start(mut self, mut read: tokio::sync::mpsc::UnboundedReceiver<ChannelMessage>) {
-        if let Err(err) = self._inner.open(&self.info).await {
+        if let Err(err) = self.inner.open(&self.info).await {
             error!(%err, "Error encountered in Service::open");
         }
 
@@ -86,24 +106,22 @@ impl Channel {
                 ChannelMessage::Detach(session) => self.detach(session).await,
                 ChannelMessage::IPC(ipc) => self.message(ipc.command, ipc.session).await,
                 ChannelMessage::ProcessDead(exit_code) => {
-                    self._inner.proccess_died(&self.info, exit_code).await
+                    self.inner.proccess_died(&self.info, exit_code).await
                 }
                 ChannelMessage::Replspace(session, msg, respond) => {
-                    self._inner
+                    self.inner
                         .replspace(&self.info, msg, session, respond)
                         .await
                 }
-                ChannelMessage::Shutdown => match self._inner.shutdown(&self.info).await {
-                    Ok(_) => break,
+                ChannelMessage::Shutdown => match self.inner.shutdown(&self.info).await {
+                    Ok(()) => break,
                     Err(err) => {
                         error!(%err, "Error encountered in Service#shutdown");
                         break;
                     }
                 },
-                ChannelMessage::FSEvent(event) => self._inner.fsevent(&self.info, event).await,
-                ChannelMessage::ExternalMessage(msg, sessions) => {
-                    self.info.send(msg, sessions).await
-                }
+                ChannelMessage::FSEvent(event) => self.inner.fsevent(&self.info, event).await,
+                ChannelMessage::ExternalMessage(msg, sessions) => self.info.send(msg, sessions),
             };
 
             if let Err(err) = result {
@@ -117,12 +135,12 @@ impl Channel {
 impl Channel {
     async fn message(&mut self, message: goval::Command, session: SessionID) -> Result<()> {
         if let Some(mut msg) = self
-            ._inner
+            .inner
             .message(&self.info, message.clone(), session)
             .await?
         {
             msg.r#ref = message.r#ref;
-            self.info.send(msg, SendSessions::Only(session)).await?
+            self.info.send(msg, SendSessions::Only(session))?;
         }
 
         Ok(())
@@ -137,13 +155,13 @@ impl Channel {
         self.info.sessions.insert(session, client.clone());
         self.info.clients.insert(session, sender.clone());
         match self
-            ._inner
+            .inner
             .attach(&self.info, client, session, sender)
             .await?
         {
             None => {}
             Some(msg) => {
-                self.info.send(msg, SendSessions::Only(session)).await?;
+                self.info.send(msg, SendSessions::Only(session))?;
             }
         }
         Ok(())
@@ -152,7 +170,7 @@ impl Channel {
     async fn detach(&mut self, session: SessionID) -> Result<()> {
         self.info.sessions.retain(|sess, _| sess != &session);
         self.info.clients.retain(|sess, _| sess != &session);
-        self._inner.detach(&self.info, session).await?;
+        self.inner.detach(&self.info, session).await?;
         Ok(())
     }
 }