fix: preserve decoration when inserting dependency

crates/pixi_core/src/workspace/workspace_mut.rs

@@ -288,7 +288,7 @@ impl WorkspaceMut {
                 feature_name,
                 Some(editable),
                 DependencyOverwriteBehavior::Overwrite,
-                location.as_ref(),
+                location,
             )?;
             if added {
                 if spec.version_or_url.is_none() {
@@ -623,7 +623,7 @@ impl WorkspaceMut {
                     feature_name,
                     Some(editable),
                     DependencyOverwriteBehavior::Overwrite,
-                    location.as_ref(),
+                    location,
                 )?;
             }
         }

crates/pixi_manifest/src/lib.rs

@@ -89,6 +89,7 @@ pub enum DependencyOverwriteBehavior {
     Error,
 }
 
+#[derive(Copy, Clone)]
 pub enum PypiDependencyLocation {
     /// [pypi-dependencies] in pixi.toml or [tool.pixi.pypi-dependencies] in
     /// pyproject.toml

crates/pixi_manifest/src/manifests/document.rs

@@ -1,10 +1,5 @@
 use std::{fmt, str::FromStr, sync::Arc};
 
-use crate::{
-    FeatureName, LibCSystemRequirement, ManifestKind, ManifestProvenance, PypiDependencyLocation,
-    SpecType, SystemRequirements, Task, TomlError, manifests::table_name::TableName,
-    toml::TomlDocument, utils::WithSourceCode,
-};
 use miette::{Diagnostic, NamedSource};
 use pixi_consts::consts;
 use pixi_pypi_spec::{PixiPypiSpec, PypiPackageName};
@@ -13,6 +8,12 @@ use rattler_conda_types::{PackageName, Platform};
 use thiserror::Error;
 use toml_edit::{Array, DocumentMut, Item, Table, Value, value};
 
+use crate::{
+    FeatureName, LibCSystemRequirement, ManifestKind, ManifestProvenance, PypiDependencyLocation,
+    SpecType, SystemRequirements, Task, TomlError, manifests::table_name::TableName,
+    toml::TomlDocument, utils::WithSourceCode,
+};
+
 /// Discriminates between a 'pixi.toml' and a 'pyproject.toml' manifest.
 #[derive(Debug, Clone)]
 pub enum ManifestDocument {
@@ -245,39 +246,14 @@ impl ManifestDocument {
     ) -> Result<(), TomlError> {
         // For 'pyproject.toml' manifest, try and remove the dependency from native
         // arrays
-        let remove_requirement =
-            |source: &mut ManifestDocument, table, array_name| -> Result<(), TomlError> {
-                let array = source
-                    .manifest_mut()
-                    .get_mut_toml_array(table, array_name)?;
-                if let Some(array) = array {
-                    array.retain(|x| {
-                        let req: pep508_rs::Requirement = x
-                            .as_str()
-                            .unwrap_or("")
-                            .parse()
-                            .expect("should be a valid pep508 dependency");
-                        let name = PypiPackageName::from_normalized(req.name);
-                        name != *dep
-                    });
-                    if array.is_empty() {
-                        source
-                            .manifest_mut()
-                            .get_or_insert_nested_table(table)?
-                            .remove(array_name);
-                    }
-                }
-                Ok(())
-            };
-
         match self {
             ManifestDocument::PyProjectToml(_) if feature_name.is_default() => {
-                remove_requirement(self, "project", "dependencies")?;
+                self.remove_pypi_requirement("project", "dependencies", dep)?;
             }
             ManifestDocument::PyProjectToml(_) => {
                 let name = feature_name.to_string();
-                remove_requirement(self, "project.optional-dependencies", &name)?;
-                remove_requirement(self, "dependency-groups", &name)?;
+                self.remove_pypi_requirement("project.optional-dependencies", &name, dep)?;
+                self.remove_pypi_requirement("dependency-groups", &name, dep)?;
             }
             _ => (),
         };
@@ -296,6 +272,33 @@ impl ManifestDocument {
         Ok(())
     }
 
+    /// Removes a pypi requirement from a particular array.
+    fn remove_pypi_requirement(
+        &mut self,
+        table: &str,
+        array_name: &str,
+        dependency_name: &PypiPackageName,
+    ) -> Result<(), TomlError> {
+        let array = self.manifest_mut().get_mut_toml_array(table, array_name)?;
+        if let Some(array) = array {
+            array.retain(|x| {
+                let req: pep508_rs::Requirement = x
+                    .as_str()
+                    .unwrap_or("")
+                    .parse()
+                    .expect("should be a valid pep508 dependency");
+                let name = PypiPackageName::from_normalized(req.name);
+                name != *dependency_name
+            });
+            if array.is_empty() {
+                self.manifest_mut()
+                    .get_or_insert_nested_table(table)?
+                    .remove(array_name);
+            }
+        }
+        Ok(())
+    }
+
     /// Removes a conda or pypi dependency from the TOML manifest's pixi table
     /// for either a 'pyproject.toml' and 'pixi.toml'
     ///
@@ -338,7 +341,30 @@ impl ManifestDocument {
 
         self.manifest_mut()
             .get_or_insert_nested_table(dependency_table.to_string().as_str())
-            .map(|t| t.insert(name.as_normalized(), Item::Value(spec.to_toml_value())))?;
+            .map(|t| {
+                let mut new_value = spec.to_toml_value();
+
+                // Check if there is an existing entry that is represented by an inline value.
+                let existing_value = t.iter_mut().find_map(|(key, value)| {
+                    let package_key_name = PackageName::from_str(key.get()).ok()?;
+                    if package_key_name == *name {
+                        value.as_value_mut()
+                    } else {
+                        None
+                    }
+                });
+
+                // If there exists an existing value, we update it with the new value, but we
+                // keep the decoration.
+                if let Some(existing_value) = existing_value {
+                    *new_value.decor_mut() = existing_value.decor().clone();
+                    *existing_value = new_value;
+                } else {
+                    // Otherwise, just reinsert the value. This might overwrite an existing
+                    // decorations.
+                    t.insert(name.as_normalized(), Item::Value(new_value));
+                }
+            })?;
 
         Ok(())
     }
@@ -354,19 +380,8 @@ impl ManifestDocument {
         platform: Option<Platform>,
         feature_name: &FeatureName,
         editable: Option<bool>,
-        location: Option<&PypiDependencyLocation>,
+        location: Option<PypiDependencyLocation>,
     ) -> Result<(), TomlError> {
-        // Pypi dependencies can be stored in different places in pyproject.toml
-        // manifests so we remove any potential dependency of the same name
-        // before adding it back
-        if matches!(self, ManifestDocument::PyProjectToml(_)) {
-            self.remove_pypi_dependency(
-                &PypiPackageName::from_normalized(requirement.name.clone()),
-                platform,
-                feature_name,
-            )?;
-        }
-
         // The '[pypi-dependencies]' or '[tool.pixi.pypi-dependencies]' table is
         // selected
         //  - For 'pixi.toml' manifests where it is the only choice
@@ -385,18 +400,44 @@ impl ManifestDocument {
                 pypi_requirement.set_editable(editable);
             }
 
-            let dependency_table = TableName::new()
+            let dependency_table_name = TableName::new()
                 .with_prefix(self.table_prefix())
                 .with_platform(platform.as_ref())
                 .with_feature_name(Some(feature_name))
                 .with_table(Some(consts::PYPI_DEPENDENCIES));
 
-            self.manifest_mut()
-                .get_or_insert_nested_table(dependency_table.to_string().as_str())?
-                .insert(
-                    requirement.name.as_ref(),
-                    Item::Value(pypi_requirement.into()),
-                );
+            let table = self
+                .manifest_mut()
+                .get_or_insert_nested_table(dependency_table_name.to_string().as_str())?;
+
+            let mut new_value = Value::from(pypi_requirement);
+
+            // Check if there exists an existing entry in the table that we should overwrite
+            // instead.
+            let existing_value = table.iter_mut().find_map(|(key, value)| {
+                let existing_name = pep508_rs::PackageName::from_str(key.get()).ok()?;
+                if existing_name == requirement.name {
+                    value.as_value_mut()
+                } else {
+                    None
+                }
+            });
+
+            // If there exists an existing entry, we overwrite it but keep the decoration.
+            if let Some(existing_value) = existing_value {
+                *new_value.decor_mut() = existing_value.decor().clone();
+                *existing_value = new_value;
+            } else {
+                table.insert(requirement.name.as_ref(), Item::Value(new_value));
+            }
+
+            // Remove the entry from the project native array.
+            self.remove_pypi_requirement(
+                "project",
+                "dependencies",
+                &PypiPackageName::from_normalized(requirement.name.clone()),
+            )?;
+
             return Ok(());
         }
 
@@ -406,10 +447,26 @@ impl ManifestDocument {
         //   - optional-dependencies is explicitly requested as location
         let add_requirement =
             |source: &mut ManifestDocument, table, array| -> Result<(), TomlError> {
-                source
+                let array = source
                     .manifest_mut()
-                    .get_or_insert_toml_array_mut(table, array)?
-                    .push(requirement.to_string());
+                    .get_or_insert_toml_array_mut(table, array)?;
+
+                // Check if there is an existing entry that we should replace. Replacing will
+                // preserve any existing formatting.
+                let existing_entry_idx = array.iter().position(|item| {
+                    let Ok(req): Result<pep508_rs::Requirement, _> =
+                        item.as_str().unwrap_or_default().parse()
+                    else {
+                        return false;
+                    };
+                    req.name == requirement.name
+                });
+
+                if let Some(idx) = existing_entry_idx {
+                    array.replace(idx, requirement.to_string());
+                } else {
+                    array.push(requirement.to_string());
+                }
                 Ok(())
             };
         if feature_name.is_default()
@@ -749,3 +806,125 @@ impl ManifestDocument {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    /// This test checks that when calling `add_dependency` with a dependency
+    /// that already exists in the document the formatting and comments are
+    /// preserved.
+    ///
+    /// This test also verifies that the name of the dependency is correctly
+    /// found and kept in the situation where the user has a non-normalized name
+    /// in the document (e.g. "hTTPx" vs 'HtTpX').
+    #[test]
+    pub fn add_dependency_retains_decoration() {
+        let manifest_content = r#"[project]
+name = "test"
+
+[tool.pixi.project]
+channels = []
+platforms = []
+
+[tool.pixi.dependencies]
+# Hello world!
+hTTPx = ">=0.28.1,<0.29" # Some comment.
+
+# newline
+"#;
+
+        let mut document = ManifestDocument::PyProjectToml(TomlDocument::new(
+            DocumentMut::from_str(manifest_content).unwrap(),
+        ));
+
+        // Overwrite the existing dependency.
+        document
+            .add_dependency(
+                &PackageName::from_str("HtTpX").unwrap(),
+                &PixiSpec::Version("0.1.*".parse().unwrap()),
+                SpecType::Run,
+                None,
+                &FeatureName::default(),
+            )
+            .unwrap();
+
+        insta::assert_snapshot!(document.to_string());
+    }
+
+    /// This test checks that when calling `add_pypi_dependency` with
+    /// dependencies that already exist in different locations the
+    /// formatting and comments are preserved across all PyPI dependency
+    /// storage formats.
+    ///
+    /// This test also verifies that the name of the dependency is correctly
+    /// found and kept in the situation where the user has a non-normalized name
+    /// in the document (e.g. "rEquEsTs" vs 'ReQuEsTs').
+    #[test]
+    pub fn add_pypi_dependency_retains_decoration() {
+        let manifest_content = r#"[project]
+name = "test"
+dependencies = [
+    # Main dependency comment
+    "rEquEsTs>=2.28.1,<3.0", # inline comment
+]
+
+[project.optional-dependencies]
+dev = [
+    # Dev dependency comment  
+    "PyYaML>=6.0", # dev inline comment
+]
+
+[tool.pixi.project]
+channels = []
+platforms = []
+
+[tool.pixi.pypi-dependencies]
+# Table dependency comment
+NumPy = ">=1.20.0" # table inline comment
+"#;
+
+        let mut document = ManifestDocument::PyProjectToml(TomlDocument::new(
+            DocumentMut::from_str(manifest_content).unwrap(),
+        ));
+
+        // Update dependencies in different locations
+        let requests_req = pep508_rs::Requirement::from_str("ReQuEsTs>=2.30.0").unwrap();
+        document
+            .add_pypi_dependency(
+                &requests_req,
+                None,
+                None,
+                &FeatureName::default(),
+                None,
+                Some(PypiDependencyLocation::Dependencies),
+            )
+            .unwrap();
+
+        let pyyaml_req = pep508_rs::Requirement::from_str("PyYAML>=6.1.0").unwrap();
+        document
+            .add_pypi_dependency(
+                &pyyaml_req,
+                None,
+                None,
+                &FeatureName::from_str("dev").unwrap(),
+                None,
+                Some(PypiDependencyLocation::OptionalDependencies),
+            )
+            .unwrap();
+
+        let numpy_req = pep508_rs::Requirement::from_str("numpy>=1.21.0").unwrap();
+        document
+            .add_pypi_dependency(
+                &numpy_req,
+                None,
+                None,
+                &FeatureName::default(),
+                None,
+                Some(crate::PypiDependencyLocation::PixiPypiDependencies),
+            )
+            .unwrap();
+
+        insta::assert_snapshot!(document.to_string());
+    }
+}