[no ci] review progress

Author: mdgeorge4153

external-crates/move/crates/move-package-alt/TODO.md

@@ -1,7 +1,18 @@
- - [ ] tests/test_runner.rs
- - [ ] dependency/dependency_set.rs
- - [ ] dependency/mod.rs
- - [ ] dependency/local.rs
+TODO
+====
+ - in many instances we've wanted to do some amount of tranformation when converting from serialized
+   stuff into our data structures. I've been thinking for a while that we may want to just have a
+   separate parsing module that contains only the parsing data structures and
+   serialization/deserialization code. We've been avoiding this for a while and I think it's
+   causing us headaches
+
+ -
+
+Needs review
+============
+ - [X] dependency/dependency_set.rs
+ - [X] dependency/mod.rs
+ - [X] dependency/local.rs
  - [ ] dependency/git.rs
 
  - [ ] dependency/external.rs
@@ -33,6 +44,7 @@
  - [ ] package/lockfile.rs
  - [ ] package/manifest.rs
 
+ - [ ] tests/test_runner.rs
  - [ ] tests/data/flavor_sui/Move@parsed.snap
  - [ ] tests/data/flavor_sui/Move.parsed
  - [ ] tests/data/flavor_sui/Move.toml

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

@@ -14,6 +14,12 @@ use serde::{Deserialize, Serialize};
 
 use crate::package::{EnvironmentName, PackageName};
 
+// TODO (potential refactor): using [Option] here and representing the default environment as
+// [None] has led to some confusion, it might be better to make a specific enumeration, so that
+// [DependencySet] becomes closer to a vanilla map. In fact, in the dependency graph we'll have
+// `Option<EnvironmentName>, PackageName` edges - this might be a good type to encapsulate; then
+// DependencySet just becomes a map from edges to T. A little curry can go a long way
+
 /// A set of default dependencies and dep overrides. Within each environment, package names are
 /// unique.
 ///
@@ -262,6 +268,7 @@ impl<T> FromIterator<(Option<EnvironmentName>, PackageName, T)> for DependencySe
     }
 }
 
+// TODO: maybe this can be derived with our fancy derive macros?
 // Note: can't be derived because that adds a spurious T: Default bound
 impl<T> Default for DependencySet<T> {
     /// The empty dependency set

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

@@ -6,6 +6,7 @@
 //!
 //! Git dependencies are cached in `~/.move`, which has the following structure:
 //!
+//! TODO: this doesn't match the implementation below:
 //! ```ignore
 //! .move/
 //!   git/
@@ -34,6 +35,13 @@ use crate::errors::{GitError, GitErrorKind, Located, PackageError, PackageResult
 
 use super::{DependencySet, Pinned, Unpinned};
 
+// TODO: (potential refactor): it might be good to separate out a separate module that is just git
+//       stuff and another that uses that git stuff to implement the dependency operations (like
+//       the jsonrpc / dependency::external split).
+
+// 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 = ""`
 type Sha = String;
 
 /// A git dependency that is unpinned. The `rev` field can be either empty, a branch, or a sha. To
@@ -60,7 +68,6 @@ pub struct PinnedGitDependency {
     repo: String,
 
     /// The exact sha for the revision
-    // rev: Sha,
     #[serde(deserialize_with = "deserialize_sha")]
     rev: Sha,
 
@@ -81,7 +88,8 @@ pub struct GitRepo {
     path: PathBuf,
 }
 
-// Custom error type for SHA validation
+/// Custom error type for SHA validation
+// TODO: derive(Error)?
 #[derive(Debug)]
 pub enum ShaError {
     InvalidLength(usize),

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

@@ -16,6 +16,7 @@ pub struct LocalDependency {
     local: PathBuf,
 }
 
+// TODO: dead code
 impl TryFrom<(&Path, toml_edit::Value)> for LocalDependency {
     type Error = anyhow::Error; // TODO
 

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

@@ -2,28 +2,22 @@
 // Copyright (c) The Move Contributors
 // SPDX-License-Identifier: Apache-2.0
 
-mod dependency_set;
-// TODO: this shouldn't be pub; need to move resolver error into resolver module
-pub mod external;
-mod git;
-mod local;
-
-pub use dependency_set::DependencySet;
 use std::{
     collections::BTreeMap,
     fmt::{self, Debug},
     marker::PhantomData,
     path::PathBuf,
     process::{Command, Stdio},
 };
-use tracing::debug;
 
 use derive_where::derive_where;
 use serde::{
     Deserialize, Deserializer, Serialize,
     de::{self, MapAccess, SeqAccess, Visitor},
 };
 
+use tracing::debug;
+
 use crate::{
     errors::{GitError, PackageError, PackageResult, ResolverError},
     flavor::MoveFlavor,
@@ -34,6 +28,24 @@ use external::ExternalDependency;
 use git::{GitRepo, PinnedGitDependency, UnpinnedGitDependency};
 use local::LocalDependency;
 
+mod dependency_set;
+// TODO: this shouldn't be pub; need to move resolver error into resolver module
+pub mod external;
+mod git;
+mod local;
+
+pub use dependency_set::DependencySet;
+
+// TODO (potential refactor): consider using objects for manifest dependencies (i.e. `Box<dyn UnpinnedDependency>`).
+//      part of the complexity here would be deserialization - probably need a flavor-specific
+//      function that converts a toml value to a Box<dyn UnpinnedDependency>
+//
+//      resolution would also be interesting because of batch resolution. Would probably need a
+//      trait method to return a resolver object, and then a method on the resolver object to
+//      resolve a bunch of dependencies (resolvers could implement Eq)
+//
+// TODO: maybe rename ManifestDependencyInfo to UnpinnedDependency
+
 /// Phantom type to represent pinned dependencies (see [PinnedDependency])
 #[derive(Debug, PartialEq, Eq)]
 pub struct Pinned;
@@ -45,6 +57,7 @@ pub struct Unpinned;
 /// [ManifestDependencyInfo]s contain the dependency-type-specific things that users write in their
 /// Move.toml files in the `dependencies` section.
 ///
+/// TODO: this paragraph will change with upcoming design changes:
 /// There are additional general fields in the manifest format (like `override` or `rename-from`)
 /// that are not part of the ManifestDependencyInfo. We separate these partly because these things
 /// are not serialized to the Lock file. See [crate::package::manifest] for the full representation
@@ -80,6 +93,8 @@ pub enum PinnedDependencyInfo<F: MoveFlavor + ?Sized> {
     FlavorSpecific(F::FlavorDependency<Pinned>),
 }
 
+// TODO: these should be moved down.
+
 // UNPINNED
 impl<'de, F> Deserialize<'de> for ManifestDependencyInfo<F>
 where
@@ -184,6 +199,7 @@ fn split<F: MoveFlavor>(
     (gits, exts, locs, flav)
 }
 
+// TODO: this will change with upcoming design changes:
 /// Replace all dependencies with their pinned versions. The returned set may have a different set
 /// of keys than the input, for example if new implicit dependencies are added or if external
 /// resolvers resolve default deps to dep-overrides, or if dep-overrides are identical to the
@@ -231,6 +247,7 @@ pub async fn pin<F: MoveFlavor>(
     ]))
 }
 
+// TODO: this will change with the upcoming design changes:
 /// For each environment, if none of the implicit dependencies are present in [deps] (or the
 /// default environment), then they are all added.
 // TODO: what's the notion of identity used here?
@@ -250,3 +267,7 @@ fn fetch<F: MoveFlavor>(
 
     todo!()
 }
+
+// TODO: unit tests
+#[cfg(test)]
+mod tests {}