fix WorkspaceSource's issues with rename and default-features #10697

Muscraft · 2022-05-23T19:33:40Z

Motivations and Context

As talked about in #10685, there were some issues with the cargo_add::Dependency methods. They revolved around setting a source to a WorkspaceSource from another Source. They would leave remnants of the other source that conflicts with a WorkspaceSource (rename and default-features). This fixes that issue by making sure you cannot set rename or default-features if the source is a WorkspaceSource. Additionally, it clears the conflicting fields in a Dependency so they cannot show up even if you switched the Source.

Changes

Made set_rename and set_default_features return a CargoResult
- They also return an error if the method is called on a WorkspaceSource
Made set_source clear conflicting fields when called with a WorkspaceSource

r? @epage

epage

My first thought with this is: is this important enough that we should be focusing on this right now?

epage · 2022-05-23T19:53:31Z

src/cargo/ops/cargo_add/dependency.rs

+        if let Source::Workspace(_) = source {
+            self.default_features = None;
+            self.rename = None;
+        }
        self.source = Some(source.into());


This .into() isn't needed

epage · 2022-05-23T19:54:40Z

src/cargo/ops/cargo_add/dependency.rs

+        let source = source.into();
+        if let Source::Workspace(_) = source {
+            self.default_features = None;
+            self.rename = None;


This clears rename which means that toml_key will change when setting the source.

Is that what we should expect?

I debated heavily between clearing the fields or using bail!. I decided against clearing the fields as if you want to set a new source the idea is that you do not care about those fields. I can see how a CargoResult<Self> could be better and that should be an easy change to make if you feel it is better

This left out another options: move rename into name so that the dependency table is as if the package field was dropped. I was wondering about that vs what is here

epage · 2022-05-23T19:55:19Z

src/cargo/ops/cargo_add/dependency.rs

-            if self.source != Some(Source::Workspace(WorkspaceSource)) {
-                key.fmt();
-            }


This is in the same commit as everything else without an explanation

There was a problem with formatting after removal and this fixed it. Do you think it would be better in a separate commit or described in the PR?

Separate PR would mean its not blocked on everything else

epage · 2022-05-23T19:59:20Z

src/cargo/ops/cargo_add/dependency.rs

@@ -1047,12 +1094,12 @@ mod tests {
    }

    #[test]
-    fn to_toml_renamed_dep() {
+    fn to_toml_renamed_dep() -> CargoResult<()> {


What does it take for this to show a stack trace? If the experience is sub-par wrt backtraces, I'd just unwrap instead

I'm not entirely sure what it would take, so I will change it to unwrap

epage · 2022-05-23T20:00:05Z

src/cargo/ops/cargo_add/mod.rs

@@ -565,7 +566,7 @@ fn populate_dependency(mut dependency: Dependency, arg: &DepOp) -> Dependency {
    }

    if let Some(rename) = &arg.rename {
-        dependency = dependency.set_rename(rename);
+        dependency = dependency.set_rename(rename).unwrap();


If you have a reason to believe an unwrap is justified, it should be documented, ideally as an except. Otherwise, chain the error

epage · 2022-05-23T20:01:30Z

src/cargo/ops/cargo_add/mod.rs

@@ -284,6 +284,7 @@ fn resolve_dependency(
        // Checking for a workspace dependency happens first since a member could be specified
        // in the workspace dependencies table as a dependency
        if let Some(_dep) = find_workspace_dep(dependency.toml_key(), ws.root_manifest()).ok() {
+            check_invalid_ws_keys(dependency.toml_key(), arg)?;


What is the reason this was added? This is done in the same commit as everything else but its not clear how its related?

I missed adding this to the PR, if this was not here it changes the error message. It prints the error message according to the toml_key which changes in the next line (potentially)

epage · 2022-05-23T20:02:03Z

src/cargo/ops/cargo_add/dependency.rs

+                if let Some(inherit_features) = &self.inherited_features {
+                    inherit_features.iter().for_each(|f| {
+                        features.remove(f.as_str());
+                    });
+                }
+                if !features.is_empty() {
+                    let features =
+                        toml_edit::value(features.into_iter().collect::<toml_edit::Value>());
+                    table.set_dotted(false);
+                    table.insert("features", features);
+                } else {
+                    table.remove("features");
+                }


Not quite convinced this is the right route to go. inherit_features is just advisory before this change

This is my bad, I accidentally added this. It should've been moved to my shelf

Muscraft · 2022-05-23T20:16:56Z

My first thought with this is: is this important enough that we should be focusing on this right now?

I could go either way on the importance of this right now. I think it is a change that should be made at some point but it is not needed right at this moment. If it should be closed until a better time just let me know!

epage · 2022-05-26T13:51:17Z

I could go either way on the importance of this right now. I think it is a change that should be made at some point but it is not needed right at this moment. If it should be closed until a better time just let me know!

For me, the issue is this jumps straight to solving it one way when there are multiple ways without any discussion of the trade offs. Having the discussion on which way to go was what I didn't feel was important enough to have right now.

Muscraft · 2022-05-26T19:01:50Z

That works for me, I'll close this and open a PR for the key formatting issue!

fix key formatting when switching to a dotted `WorkspaceSource` This fell out of #10697 see [this comment](#10697 (comment)) There was a small issue where changing the source of a `cargo_add::Dependency` to a `WorkspaceSource` would cause the dotted version to have extra space. ```toml dep .workspace = true dep.workspace = true ``` This makes sure the key is formatted as well as adds a unit test to make sure this doesn't come back up in the future. r? `@epage`

fix WorkspaceSource's issues with rename and default-features

29f7c37

rust-highfive assigned epage May 23, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2022

epage reviewed May 23, 2022

View reviewed changes

Muscraft closed this May 26, 2022

Muscraft mentioned this pull request May 26, 2022

fix key formatting when switching to a dotted WorkspaceSource #10705

Merged

Muscraft deleted the cargo-add-workspace-source branch August 17, 2022 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix WorkspaceSource's issues with rename and default-features #10697

fix WorkspaceSource's issues with rename and default-features #10697

Muscraft commented May 23, 2022

epage left a comment

epage May 23, 2022

epage May 23, 2022

Muscraft May 23, 2022

epage May 26, 2022

epage May 23, 2022

Muscraft May 23, 2022

epage May 26, 2022

epage May 23, 2022

Muscraft May 23, 2022

epage May 23, 2022

epage May 23, 2022

Muscraft May 23, 2022

epage May 23, 2022

Muscraft May 23, 2022

Muscraft commented May 23, 2022

epage commented May 26, 2022

Muscraft commented May 26, 2022

fix WorkspaceSource's issues with rename and default-features #10697

fix WorkspaceSource's issues with rename and default-features #10697

Conversation

Muscraft commented May 23, 2022

Motivations and Context

Changes

epage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Muscraft commented May 23, 2022

epage commented May 26, 2022

Muscraft commented May 26, 2022