Review of package-alt

Author: mdgeorge4153

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

@@ -0,0 +1,181 @@
+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
+ - [X] dependency/git.rs - will rereview with cleanup PR
+
+ - [ ] dependency/external.rs
+ - [ ] mocks/mock-resolver.rs
+ - [ ] jsonrpc/types.rs
+ - [ ] jsonrpc/client.rs
+ - [ ] jsonrpc/mod.rs
+ - [ ] tests/jsonrpc_tests.rs
+
+
+ - [ ] errors/manifest_error.rs
+ - [ ] errors/git_error.rs
+ - [ ] errors/files.rs
+ - [ ] errors/mod.rs
+ - [ ] errors/located.rs
+ - [ ] errors/lockfile_error.rs
+ - [ ] errors/resolver_error.rs
+ - [ ] lib.rs
+
+ - [ ] main.rs
+ - [ ] cli/build.rs
+ - [ ] cli/mod.rs
+ - [ ] cli/parse.rs
+
+ - [ ] package/mod.rs
+ - [ ] flavor/vanilla.rs
+ - [ ] flavor/mod.rs
+
+ - [ ] 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
+ - [ ] tests/data/edition_unknown/Move@parsed.snap
+ - [ ] tests/data/edition_unknown/Move.parsed
+ - [ ] tests/data/edition_unknown/Move.toml
+ - [ ] tests/data/edition_2024_alpha/Move@parsed.snap
+ - [ ] tests/data/edition_2024_alpha/Move.parsed
+ - [ ] tests/data/edition_2024_alpha/Move.toml
+ - [ ] tests/data/external_returns_external/Move.pinned
+ - [ ] tests/data/external_returns_external/Move@pinned.snap
+ - [ ] tests/data/external_returns_external/Move.toml
+ - [ ] tests/data/too_many_entries_renaming_instantiation/Move@parsed.snap
+ - [ ] tests/data/too_many_entries_renaming_instantiation/Move.parsed
+ - [ ] tests/data/too_many_entries_renaming_instantiation/Move.toml
+ - [ ] tests/data/invalid_lockfile_parsing/Move.snap
+ - [ ] tests/data/no_path_set_for_dependency/Move@parsed.snap
+ - [ ] tests/data/no_path_set_for_dependency/Move.parsed
+ - [ ] tests/data/no_path_set_for_dependency/Move.toml
+ - [ ] tests/data/flavor_unknown/Move@parsed.snap
+ - [ ] tests/data/flavor_unknown/Move.parsed
+ - [ ] tests/data/flavor_unknown/Move.toml
+ - [ ] tests/data/invalid_identifier_package_name/Move@parsed.snap
+ - [ ] tests/data/invalid_identifier_package_name/Move.parsed
+ - [ ] tests/data/invalid_identifier_package_name/Move.toml
+ - [ ] tests/data/missing_required_package_segment/Move@parsed.snap
+ - [ ] tests/data/missing_required_package_segment/Move.parsed
+ - [ ] tests/data/missing_required_package_segment/Move.toml
+ - [ ] tests/data/missing_minimal_required_field_name/Move@parsed.snap
+ - [ ] tests/data/missing_minimal_required_field_name/Move.parsed
+ - [ ] tests/data/missing_minimal_required_field_name/Move.toml
+ - [ ] tests/data/basic_implicit_deps/Move@parsed.snap
+ - [ ] tests/data/basic_implicit_deps/Move.parsed
+ - [ ] tests/data/basic_implicit_deps/Move.toml
+ - [ ] tests/data/invalid_authors/Move@parsed.snap
+ - [ ] tests/data/invalid_authors/Move.parsed
+ - [ ] tests/data/invalid_authors/Move.toml
+ - [ ] tests/data/lockfile_parsing_valid/Move.toml
+ - [ ] tests/data/lockfile_parsing_valid/Move@locked.snap
+ - [ ] tests/data/lockfile_parsing_valid/Move.lock
+ - [ ] tests/data/lockfile_parsing_valid/Move.locked
+ - [ ] tests/data/edition_unknown_suffix/Move@parsed.snap
+ - [ ] tests/data/edition_unknown_suffix/Move.parsed
+ - [ ] tests/data/edition_unknown_suffix/Move.toml
+ - [ ] tests/data/flavor_global_storage/Move@parsed.snap
+ - [ ] tests/data/flavor_global_storage/Move.parsed
+ - [ ] tests/data/flavor_global_storage/Move.toml
+ - [ ] tests/data/missing_required_envs_segment/Move.snap
+ - [ ] tests/data/missing_required_envs_segment/Move.toml
+ - [ ] tests/data/external_multiple_resolvers/Move.pinned
+ - [ ] tests/data/external_multiple_resolvers/Move@pinned.snap
+ - [ ] tests/data/external_multiple_resolvers/Move.toml
+ - [ ] tests/data/non_identifier_address_name_in_subst/Move@parsed.snap
+ - [ ] tests/data/non_identifier_address_name_in_subst/Move.parsed
+ - [ ] tests/data/non_identifier_address_name_in_subst/Move.toml
+ - [ ] tests/data/basic/Move@parsed.snap
+ - [ ] tests/data/basic/Move.parsed
+ - [ ] tests/data/basic/Move.toml
+ - [ ] tests/data/external_missing_keys/Move.pinned
+ - [ ] tests/data/external_missing_keys/Move@pinned.snap
+ - [ ] tests/data/external_missing_keys/Move.toml
+ - [ ] tests/data/package_name_empty/Move@parsed.snap
+ - [ ] tests/data/package_name_empty/Move.parsed
+ - [ ] tests/data/package_name_empty/Move.toml
+ - [ ] tests/data/external_empty_output/Move.pinned
+ - [ ] tests/data/external_empty_output/Move@pinned.snap
+ - [ ] tests/data/external_empty_output/Move.toml
+ - [ ] tests/data/edition_legacy/Move@parsed.snap
+ - [ ] tests/data/edition_legacy/Move.parsed
+ - [ ] tests/data/edition_legacy/Move.toml
+ - [ ] tests/data/invalid_environment2/Move@parsed.snap
+ - [ ] tests/data/invalid_environment2/Move.parsed
+ - [ ] tests/data/invalid_environment2/Move.toml
+ - [ ] tests/data/external_basic/Move@pinned.snap
+ - [ ] tests/data/external_basic/Move.toml
+ - [ ] tests/data/unknown_toplevel_field/Move@parsed.snap
+ - [ ] tests/data/unknown_toplevel_field/Move.parsed
+ - [ ] tests/data/unknown_toplevel_field/Move.toml
+ - [ ] tests/data/edition_2024_beta/Move@parsed.snap
+ - [ ] tests/data/edition_2024_beta/Move.parsed
+ - [ ] tests/data/edition_2024_beta/Move.toml
+ - [ ] tests/data/non_string_package_name/Move@parsed.snap
+ - [ ] tests/data/non_string_package_name/Move.parsed
+ - [ ] tests/data/non_string_package_name/Move.toml
+ - [ ] tests/data/external_errorcode/Move.pinned
+ - [ ] tests/data/external_errorcode/Move@pinned.snap
+ - [ ] tests/data/external_errorcode/Move.toml
+ - [ ] tests/data/invalid_environment/Move@parsed.snap
+ - [ ] tests/data/invalid_environment/Move.parsed
+ - [ ] tests/data/invalid_environment/Move.toml
+ - [ ] tests/data/external_non_json/Move@pinned.snap
+ - [ ] tests/data/external_non_json/Move.toml
+ - [ ] tests/data/external_bad_schema/Move.pinned
+ - [ ] tests/data/external_bad_schema/Move@pinned.snap
+ - [ ] tests/data/external_bad_schema/Move.toml
+ - [ ] tests/data/basic_with_bar/Move@parsed.snap
+ - [ ] tests/data/basic_with_bar/Move.parsed
+ - [ ] tests/data/basic_with_bar/Move.toml
+ - [ ] tests/data/basic_with_bar/bar/Move@parsed.snap
+ - [ ] tests/data/basic_with_bar/bar/Move.parsed
+ - [ ] tests/data/basic_with_bar/bar/Move.toml
+ - [ ] tests/data/edition_2024/Move@parsed.snap
+ - [ ] tests/data/edition_2024/Move.parsed
+ - [ ] tests/data/edition_2024/Move.toml
+ - [ ] tests/data/basic_missing_git_dep/Move@parsed.snap
+ - [ ] tests/data/basic_missing_git_dep/Move.parsed
+ - [ ] tests/data/basic_missing_git_dep/Move.toml
+ - [ ] tests/data/non_string_local_dep_path/Move@parsed.snap
+ - [ ] tests/data/non_string_local_dep_path/Move.parsed
+ - [ ] tests/data/non_string_local_dep_path/Move.toml
+ - [ ] tests/data/integer_subst_in_dependency/Move@parsed.snap
+ - [ ] tests/data/integer_subst_in_dependency/Move.parsed
+ - [ ] tests/data/integer_subst_in_dependency/Move.toml
+ - [ ] tests/data/basic_move_project/config
+ - [ ] tests/data/basic_move_project/pkg_a/Move.toml
+ - [ ] tests/data/basic_move_project/pkg_b/Move.toml
+ - [ ] tests/data/invalid_hex_address_in_subst/Move@parsed.snap
+ - [ ] tests/data/invalid_hex_address_in_subst/Move.parsed
+ - [ ] tests/data/invalid_hex_address_in_subst/Move.toml
+ - [ ] tests/data/basic_no_dep_override_git/Move@parsed.snap
+ - [ ] tests/data/basic_no_dep_override_git/Move.parsed
+ - [ ] tests/data/basic_no_dep_override_git/Move.toml
+ - [ ] tests/data/duplicate_top_level_field/Move@parsed.snap
+ - [ ] tests/data/duplicate_top_level_field/Move.parsed
+ - [ ] tests/data/duplicate_top_level_field/Move.toml
+ - [ ] tests/data/invalid_environment1/Move@parsed.snap
+ - [ ] tests/data/invalid_environment1/Move.parsed
+ - [ ] tests/data/invalid_environment1/Move.toml
+ - [ ] tests/data/invalid_author/Move@parsed.snap
+ - [ ] tests/data/invalid_author/Move.parsed
+ - [ ] tests/data/invalid_author/Move.toml
+ - [ ] tests/data/basic_external_resolver/Move@parsed.snap
+ - [ ] tests/data/basic_external_resolver/Move.parsed
+ - [ ] tests/data/basic_external_resolver/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 replacements. Within each environment, package names are
 /// unique.
 ///
@@ -77,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),
@@ -127,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,
@@ -263,6 +271,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/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)]
@@ -135,6 +136,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

@@ -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,8 +35,17 @@ 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 = ""` (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)]
@@ -60,7 +70,6 @@ pub struct PinnedGitDependency {
     repo: String,
 
     /// The exact sha for the revision
-    // rev: Sha,
     #[serde(deserialize_with = "deserialize_sha")]
     rev: Sha,
 
@@ -71,6 +80,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
@@ -81,7 +91,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),
@@ -133,6 +144,7 @@ impl GitRepo {
         self.rev.as_deref()
     }
 
+    // TODO: needs a comment
     pub fn package_set_path(&self) -> &PathBuf {
         &self.path
     }
@@ -163,13 +175,16 @@ impl GitRepo {
     }
 
     /// Used for testing to be able to specify which folder to fetch to. Use `fetch` for all other needs.
+    // TODO: should be non-pub
     pub async fn fetch_to_folder(&self, fetch_to_folder: PathBuf) -> PackageResult<PathBuf> {
         self.fetch_impl(Some(fetch_to_folder)).await
     }
 
     /// 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() {
@@ -267,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],
@@ -433,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
@@ -448,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/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