Add Stefan's review comments

Author: stefan-mysten

external-crates/move/crates/move-package-alt/src/dependency/dependency_set.rs

@@ -83,6 +83,7 @@ impl<T> DependencySet<T> {
 
     /// Convenience method to return either [default_deps] or [deps_for_env] depending on [env]; an
     /// [env] of [None] indicates a request for the default dependencies.
+    /// TODO rename to deps
     pub fn deps_for(&self, env: Option<&EnvironmentName>) -> BTreeMap<PackageName, &T> {
         match env {
             Some(env) => self.deps_for_env(env),
@@ -133,6 +134,7 @@ impl<T> DependencySet<T> {
 
     /// A copy of [self] expanded with an entry (package name, env, dep) for all
     /// packages in [self] and environments in [envs].
+    /// TODO: rename to expand or extend
     pub fn explode(&mut self, envs: impl IntoIterator<Item = EnvironmentName>)
     where
         T: Clone,

external-crates/move/crates/move-package-alt/src/dependency/external.rs

@@ -60,6 +60,7 @@ struct RField {
 }
 
 /// Requests from the package mananger to the external resolver
+// TODO: fix typo above
 #[derive(Serialize, Debug)]
 struct ResolveRequest<F: MoveFlavor> {
     #[serde(default)]
@@ -134,6 +135,8 @@ impl ExternalDependency {
     }
 }
 
+// TODO: CLEANUP
+// Explain this conversion
 impl TryFrom<RField> for ExternalDependency {
     type Error = PackageError;
 

external-crates/move/crates/move-package-alt/src/dependency/git.rs

@@ -44,6 +44,8 @@ use super::{DependencySet, Pinned, Unpinned};
 //       writing `let x : Sha = ""` (whereas `let x = Sha::new("")` can fail)
 type Sha = String;
 
+/// TODO keep same style around all types
+///
 /// A git dependency that is unpinned. The `rev` field can be either empty, a branch, or a sha. To
 /// resolve this into a [`PinnedGitDependency`], call `pin_one` function.
 #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
@@ -181,6 +183,8 @@ impl GitRepo {
     /// Checkout the repository using a sparse checkout. It will try to clone without checkout, set
     /// sparse checkout directory, and then checkout the folder specified by `self.path` at the
     /// given sha.
+    ///
+    // TODO think more about debug statements and what information to log
     async fn checkout_repo(&self, repo_fs_path: &PathBuf, sha: &str) -> PackageResult<()> {
         // Checkout repo if it does not exist already
         if !repo_fs_path.exists() {
@@ -278,6 +282,7 @@ impl GitRepo {
     }
 
     /// Runs a git command from the provided arguments.
+    // TODO: check for error codes here
     pub async fn run_git_cmd_with_args(
         &self,
         args: &[&str],
@@ -444,6 +449,8 @@ pub fn format_repo_to_fs_path(repo: &str, sha: &str, root_path: Option<PathBuf>)
 
 /// Check if the given string is a valid commit SHA, i.e., 40 character long with only
 /// lowercase letters and digits
+///
+// TODO: rename this function to is_sha
 fn check_is_commit_sha(input: &str) -> bool {
     input.len() == 40
         && input
@@ -459,6 +466,7 @@ fn url_to_file_name(url: &str) -> String {
         .to_string()
 }
 
+// TODO: add more tests
 #[cfg(test)]
 mod tests {
     use super::*;

external-crates/move/crates/move-package-alt/src/jsonrpc/client.rs

@@ -18,6 +18,7 @@ pub struct Endpoint<I: AsyncRead, O: AsyncWrite> {
     sqn: RequestID,
 }
 
+// TODO: think if we keep errors here or move them in their own error module
 #[derive(Error, Debug)]
 pub enum JsonRpcError {
     #[error(transparent)]

external-crates/move/crates/move-package-alt/src/package/manifest.rs

@@ -19,6 +19,7 @@ use crate::{
 
 use super::*;
 
+// TODO: add 2025 edition
 const ALLOWED_EDITIONS: &[&str] = &["2024", "2024.beta", "legacy"];
 
 // Note: [Manifest] objects are immutable and should not implement [serde::Serialize]; any tool
@@ -94,6 +95,8 @@ impl<F: MoveFlavor> Manifest<F> {
     }
 
     /// Validate the manifest contents, after deserialization.
+    ///
+    // TODO: add more validation
     pub fn validate_manifest(&self, handle: FileHandle) -> PackageResult<()> {
         // Validate package name
         if self.package.name.get_ref().is_empty() {