Knock out some more easy TODOs

Author: jneem

cli/src/input.rs

@@ -81,7 +81,7 @@ impl<C: clap::Args + Customize> Prepare for InputOptions<C> {
         }
 
         if let Some(lock_file_path) = self.lock_file.as_ref() {
-            let lock_file = LockFile::from_path(lock_file_path);
+            let lock_file = LockFile::from_path(lock_file_path)?;
             let lock_dir = lock_file_path
                 .parent()
                 .ok_or_else(|| Error::PathWithoutParent {

package/src/error.rs

@@ -9,13 +9,19 @@ use crate::{
     UnversionedPackage,
 };
 
-// TODO: implement IntoDiagnostic.
+/// Errors related to package management.
 pub enum Error {
     Io {
         path: Option<PathBuf>,
         error: std::io::Error,
     },
-    Serialize {
+    /// We failed to serialize the index description.
+    PackageSerialization {
+        pkg: crate::index::Package,
+        error: serde_json::Error,
+    },
+    LockFileDeserialization {
+        path: PathBuf,
         error: serde_json::Error,
     },
     ManifestEval {
@@ -149,15 +155,18 @@ impl std::fmt::Display for Error {
             Error::DuplicateIndexPackageVersion { id, version } => {
                 write!(f, "package {id}@{version} is already present in the index")
             }
-            Error::Serialize { error } => {
-                write!(f, "serialization error: {error}")
+            Error::PackageSerialization { error, pkg } => {
+                write!(f, "error serializing package; this is a bug in nickel. Failed package {pkg:?}, caused by {error}")
             }
             Error::NoProjectDir => {
                 write!(
                     f,
                     "failed to find a location for the nickel cache directory"
                 )
             }
+            Error::LockFileDeserialization { path, error } => {
+                write!(f, "lock file {} is invalid: {error}", path.display())
+            }
         }
     }
 }
@@ -216,9 +225,3 @@ impl From<tempfile::PersistError> for Error {
         Self::TempFilePersist { error }
     }
 }
-
-impl From<serde_json::Error> for Error {
-    fn from(error: serde_json::Error) -> Self {
-        Self::Serialize { error }
-    }
-}

package/src/index/mod.rs

@@ -112,7 +112,10 @@ impl PackageCache {
         }
         let mut tmp = self.tmp_file(&id);
         for pkg in existing.packages.values() {
-            serde_json::to_writer(&mut tmp, pkg)?;
+            serde_json::to_writer(&mut tmp, pkg).map_err(|error| Error::PackageSerialization {
+                pkg: pkg.clone(),
+                error,
+            })?;
             tmp.write_all(b"\n").with_path(tmp.path())?;
         }
 

package/src/lib.rs

@@ -9,7 +9,7 @@ use config::Config;
 use error::Error;
 use serde::{Deserialize, Serialize};
 use serde_with::{DeserializeFromStr, SerializeDisplay};
-use version::{PartialSemVer, PartialSemVerParseError, SemVer, SemVerParseError};
+use version::{PartialSemVerParseError, SemVer, SemVerParseError, SemVerPrefix};
 
 pub mod config;
 pub mod error;
@@ -37,9 +37,7 @@ pub struct GitDependency {
 
 #[derive(Clone, Debug, PartialEq, Eq, Hash, DeserializeFromStr, SerializeDisplay)]
 pub enum VersionReq {
-    // TODO: could make this a PartialSemVer
-    Compatible(SemVer),
-    // TODO: This one could allow pre-releases
+    Compatible(SemVerPrefix),
     Exact(SemVer),
 }
 
@@ -67,17 +65,15 @@ impl FromStr for VersionReq {
         if let Some(v) = s.strip_prefix('=') {
             Ok(VersionReq::Exact(v.parse()?))
         } else {
-            Ok(VersionReq::Compatible(PartialSemVer::from_str(s)?.into()))
+            Ok(VersionReq::Compatible(SemVerPrefix::from_str(s)?))
         }
     }
 }
 
 impl VersionReq {
     pub fn matches(&self, v: &SemVer) -> bool {
         match self {
-            VersionReq::Compatible(lower_bound) => {
-                lower_bound <= v && *v < lower_bound.next_incompatible()
-            }
+            VersionReq::Compatible(lower_bound) => lower_bound.matches(v),
             VersionReq::Exact(w) => v == w,
         }
     }

package/src/lock.rs

@@ -149,10 +149,14 @@ impl LockFile {
         })
     }
 
-    // TODO: propagate the error
-    pub fn from_path(path: impl AsRef<Path>) -> Self {
-        let contents = std::fs::read_to_string(path.as_ref()).unwrap();
-        serde_json::from_str(&contents).unwrap()
+    /// Read a lock-file from disk.
+    pub fn from_path(path: impl AsRef<Path>) -> Result<Self, Error> {
+        let path = path.as_ref();
+        let contents = std::fs::read_to_string(path).with_path(path)?;
+        serde_json::from_str(&contents).map_err(|error| Error::LockFileDeserialization {
+            path: path.to_owned(),
+            error,
+        })
     }
 
     /// Build a package map from a lock-file.

package/src/manifest.rs

@@ -23,7 +23,7 @@ use crate::{
     lock::LockFile,
     repo_root,
     resolve::{Resolution, UnversionedPrecise},
-    version::{FullSemVer, PartialSemVer, SemVer},
+    version::{FullSemVer, SemVer, SemVerPrefix},
     Dependency, GitDependency, Precise, VersionReq,
 };
 
@@ -42,7 +42,7 @@ use crate::{
 struct ManifestFileFormat {
     name: Ident,
     version: FullSemVer,
-    minimal_nickel_version: PartialSemVer,
+    minimal_nickel_version: SemVerPrefix,
     dependencies: HashMap<Ident, DependencyFormat>,
 }
 

package/src/resolve.rs

@@ -163,7 +163,8 @@ impl From<BucketVersion> for SemVer {
 impl From<VersionReq> for BucketVersion {
     fn from(v: VersionReq) -> Self {
         match v {
-            VersionReq::Compatible(v) | VersionReq::Exact(v) => v.into(),
+            VersionReq::Compatible(v) => SemVer::from(v).into(),
+            VersionReq::Exact(v) => v.into(),
         }
     }
 }

package/src/version.rs

@@ -43,34 +43,19 @@ impl SemVer {
             major,
             minor,
             patch,
-            pre: String::default(),
-        }
-    }
-
-    pub fn bump_major(&self) -> SemVer {
-        SemVer {
-            major: self.major + 1,
-            minor: 0,
-            patch: 0,
-            pre: String::default(),
-        }
-    }
-
-    pub fn bump_minor(&self) -> SemVer {
-        SemVer {
-            major: self.major,
-            minor: self.minor + 1,
-            patch: 0,
-            pre: String::default(),
+            pre: String::new(),
         }
     }
+}
 
-    pub fn next_incompatible(&self) -> SemVer {
-        // TODO: should we panic or something if pre is non-empty?
-        if self.major == 0 {
-            self.bump_minor()
-        } else {
-            self.bump_major()
+impl SemVerPrefix {
+    pub fn matches(&self, v: &SemVer) -> bool {
+        match (self.minor, self.patch) {
+            (None, _) => v.major == self.major,
+            (Some(minor), None) => v.major == self.major && v.minor >= minor,
+            (Some(minor), Some(patch)) => {
+                v.major == self.major && v.minor == minor && v.patch >= patch
+            }
         }
     }
 }
@@ -113,8 +98,8 @@ impl From<SemVer> for FullSemVer {
 // information is sometimes relevant for comparing version requirements (e.g.,
 // "1.3.0" matches the requirement "1.2" but it doesn't match the requirement
 // "1.2.0").
-impl From<PartialSemVer> for SemVer {
-    fn from(psv: PartialSemVer) -> Self {
+impl From<SemVerPrefix> for SemVer {
+    fn from(psv: SemVerPrefix) -> Self {
         Self {
             major: psv.major,
             minor: psv.minor.unwrap_or(0),
@@ -166,13 +151,13 @@ impl std::fmt::Display for SemVer {
 #[derive(
     Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, serde::Serialize, serde::Deserialize,
 )]
-pub struct PartialSemVer {
+pub struct SemVerPrefix {
     pub major: u64,
     pub minor: Option<u64>,
     pub patch: Option<u64>,
 }
 
-impl PartialSemVer {
+impl SemVerPrefix {
     pub fn major_minor(major: u64, minor: u64) -> Self {
         Self {
             major,
@@ -192,7 +177,7 @@ pub enum PartialSemVerParseError {
     Num(#[from] ParseIntError),
 }
 
-impl FromStr for PartialSemVer {
+impl FromStr for SemVerPrefix {
     type Err = PartialSemVerParseError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
@@ -215,7 +200,7 @@ impl FromStr for PartialSemVer {
     }
 }
 
-impl std::fmt::Display for PartialSemVer {
+impl std::fmt::Display for SemVerPrefix {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match (self.minor, self.patch) {
             (None, _) => {