review progress

Author: mdgeorge4153

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

@@ -41,7 +41,7 @@ use super::{DependencySet, Pinned, Unpinned};
 
 // TODO: curious about the benefit of using String instead of wrapping it. The advantage of
 //       wrapping it is that we have invariants (with the type alias, nothing prevents us from
-//       writing `let x : Sha = ""`
+//       writing `let x : Sha = ""` (whereas `let x = Sha::new("")` can fail)
 type Sha = String;
 
 /// A git dependency that is unpinned. The `rev` field can be either empty, a branch, or a sha. To
@@ -78,6 +78,7 @@ pub struct PinnedGitDependency {
 
 /// Helper struct that represents a Git repository, with extra information about which folder to
 /// checkout.
+// TODO: how is this different from [UnpinnedGitDependency]?
 #[derive(Clone, Debug)]
 pub struct GitRepo {
     /// Repository URL

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

@@ -122,7 +122,10 @@ where
                 Ok(ManifestDependencyInfo::Local(dep))
             } else {
                 // TODO: maybe this could be prettier. The problem is that we don't know how to
-                // tell if something is a flavor dependency
+                // tell if something is a flavor dependency. One option might be to add a method to
+                // [MoveFlavor] that gives the list of flavor dependency tags. Another approach
+                // worth considering is removing flavor dependencies entirely and just having
+                // on-chain dependencies (with the flavor being used to resolve them).
                 let dep = toml::Value::try_from(data)
                     .map_err(de::Error::custom)?
                     .try_into()