Implement rm-override #133

teamplayer3 · 2024-09-12T19:49:24Z

resolves #130

Implementation Open Questions

use one Context for commands Override and RmOverride?
how to handle removing empty patch table? (See comment in Code)

eopb

This is fantastic! Thanks so much for working on this.

I've left some comments on some minor things, but overall this is great. There's also one test that's failing in CI, but it looks like it's just an outdated snapshot.

There are a couple of other things that would be nice to have before merging.

A test or two like the ones in tests/all/main.rs that calls cargo rm-override using assert_cmd::Command. You can copy what this function is doing.
This is useful since it tests the entire command end-to-end, not just the patch_manifest function.
A test similar to this one which tests the output of cargo rm-override --help.

Let me know if there are any parts of my review that don't make sense, or that you need help with :)

src/toml.rs

eopb · 2024-09-13T18:16:19Z

src/toml.rs

-    name: &str,
-    registry: &str,
-    mode: &context::Mode,
+    op: Operation,


nit: I'm not sure we need this Operation type. I think this could be better off with this function removed, and just using add_patch_to_manifest and remove_patch_from_manifest directly, even if the below block of code needs to be duplicated and moved into those functions

let mut manifest: toml_edit::DocumentMut = manifest .parse() .context("patch manifest contains invalid toml")?; let manifest_table = manifest.as_table_mut();

Do you have something in mind why to remove the single interface? I thought of using a struct, maybe named Manifest, which exposes the functions add_override, remove_override and perhaps later list_overrides.

I was thinking of just having a two separate independent free functions, but a struct would also work great 👍

That being said this, is a small thing so it's up to you if you'd like to change it

src/lib.rs

eopb

This is only a partial review, since I have to go AFK now. I'll review some more when I get back

Sorry that the feedback loop from CI has been slow. Once you've got commits in main, it won't need to be constantly approved to run.

eopb · 2024-09-14T17:38:08Z

src/cli.rs

@@ -17,6 +17,11 @@ pub enum CargoInvocation {
    #[command(name = "override", about)]
    #[command(next_line_help = true)]
    Override(Override),
+
+    /// Remove an override from the `[patch]` section of `Cargo.toml`s for a package.


Suggested change

/// Remove an override from the `[patch]` section of `Cargo.toml`s for a package.

/// Remove an existing override from the `[patch]` section a `Cargo.toml`.

eopb · 2024-09-14T17:40:51Z

tests/all/main.rs

@@ -25,7 +25,7 @@ use tempfile::TempDir;
 use test_case::test_case;

 #[googletest::test]
-fn patch_transative_on_regisrty() {
+fn patch_transitive_on_registry() {


Thanks for fixing these typos 🙌

eopb · 2024-09-14T17:45:08Z

src/lib.rs

-    Ok(manifest)
+    fs::write(&manifest_path, &project_manifest_toml)
+        .context("failed to write patched `Cargo.toml` file")?;
+


We should probably print something out when the removal was successful.

Something similar to what we do for adding patches.

cargo-override/src/lib.rs

Lines 161 to 164 in 7050e77

eprintln!(

"Patched dependency \"{}\" on registry \"{registry}\"",

patch_manifest.name

);

eopb · 2024-09-14T17:53:57Z

src/toml.rs

-    name: &str,
-    registry: &str,
-    mode: &context::Mode,
+    op: Operation,


I was thinking of just having a two separate independent free functions, but a struct would also work great 👍

src/context.rs

teamplayer3 · 2024-09-15T12:04:06Z

Could you check the failed tests, please? These fail because the existence of the manifest is checked before the package details are built. Is it ok to adopt the output errors in the tests, or does these tests have to be changed?

eopb · 2024-09-22T20:56:39Z

Could you check the failed tests, please? These fail because the existence of the manifest is checked before the package details are built. Is it ok to adopt the output errors in the tests, or does these tests have to be changed?

@teamplayer3, I'm ver sorry I took a whole week to responded. Last week was particularly busy at work so I didn't get many chances to look at open source. I'll also check out your other PR as soon as I get the chance.

The changes to the output look fine. Feel free to adopt the new output in the tests 👍 . Once that's done, I think I'm happy to merge

teamplayer3 · 2024-09-25T13:19:43Z

In test patch_path_doesnt_exist I have problems filtering the os error. @eopb have you an idea how to write the regex for it? Related to #147 .

eopb · 2024-09-28T14:26:52Z

In test patch_path_doesnt_exist I have problems filtering the os error. @eopb have you an idea how to write the regex for it? Related to #147 .

I think, as described in #147 (comment), we can remove snapshots that assert on cargo metadata output. assert.failure() should be enough for these cases where the output is so fragile.

teamplayer3 · 2024-09-28T23:29:29Z

I think, as described in #147 (comment), we can remove snapshots that assert on cargo metadata output. assert.failure() should be enough for these cases where the output is so fragile.

Maybe there are two points we can learn of these specific tests. First we recognize something like in #155. Secondly, the error could be too complicated and should be simplified and be implemented by cargo-override, maybe.

Yeah, it could be too picky.

teamplayer3 added 7 commits September 12, 2024 21:45

add possibility to remove a patch

9a3b214

add RmOverride command

ebfd348

take only cli::Override for conversion

42ff8c9

differentiate between the main commands and add run_rm_override

574c2cc

update tests to a more realistic example

b4d906d

refactor tests and added more variance

8ef0299

fix git string as registry gets not removed when empty

7613240

eopb requested changes Sep 13, 2024

View reviewed changes

teamplayer3 added 11 commits September 14, 2024 16:53

try to merge into one context

702a443

Add description for command rm-override

4bb5212

add short form for arg --package

874971b

fix base_help_message

dd54240

add test rm_override_subcommand_help_message

0dea785

add test scenario removing a empty patch table and an above comment

81bd622

fix spelling

0f9204e

add add_patch to Manifest

69e811b

add tests for rm-override command

f7b76b2

fix bug in ContextBuilder build

ca2163c

add extra test for comment removal

c766129

teamplayer3 requested a review from eopb September 14, 2024 16:36

teamplayer3 added 3 commits September 14, 2024 18:39

Merge commit 'dc4c853582be80176f189bb2454ab80ba1951c09' into rm-override

3c8a0be

add --package description

ed00d39

fix tests

7050e77

eopb requested changes Sep 14, 2024

View reviewed changes

src/context.rs Outdated Show resolved Hide resolved

teamplayer3 added 5 commits September 14, 2024 20:54

handle manifest operations in a struct Manifest

72a0199

fix tests rm-override

83a01b9

fixed not passing vars

c5b7fbd

remove write check

a774dd3

make clippy happy

ca2ef4f

refactor context building

aa0a26f

teamplayer3 added 2 commits September 25, 2024 15:16

fix tests

cd64a47

remove unused comments

b93dc08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement rm-override #133

Implement rm-override #133

teamplayer3 commented Sep 12, 2024 •

edited by eopb

Loading

eopb left a comment

eopb Sep 13, 2024

teamplayer3 Sep 14, 2024

eopb Sep 14, 2024

eopb Sep 14, 2024 •

edited

Loading

eopb left a comment

eopb Sep 14, 2024

eopb Sep 14, 2024

eopb Sep 14, 2024

eopb Sep 14, 2024

teamplayer3 commented Sep 15, 2024

eopb commented Sep 22, 2024

teamplayer3 commented Sep 25, 2024 •

edited

Loading

eopb commented Sep 28, 2024

teamplayer3 commented Sep 28, 2024

	/// Remove an override from the `[patch]` section of `Cargo.toml`s for a package.
	/// Remove an existing override from the `[patch]` section a `Cargo.toml`.

	eprintln!(
	"Patched dependency \"{}\" on registry \"{registry}\"",
	patch_manifest.name
	);

Implement rm-override #133

Are you sure you want to change the base?

Implement rm-override #133

Conversation

teamplayer3 commented Sep 12, 2024 • edited by eopb Loading

Implementation Open Questions

eopb left a comment

Choose a reason for hiding this comment

eopb Sep 13, 2024

Choose a reason for hiding this comment

teamplayer3 Sep 14, 2024

Choose a reason for hiding this comment

eopb Sep 14, 2024

Choose a reason for hiding this comment

eopb Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

eopb left a comment

Choose a reason for hiding this comment

eopb Sep 14, 2024

Choose a reason for hiding this comment

eopb Sep 14, 2024

Choose a reason for hiding this comment

eopb Sep 14, 2024

Choose a reason for hiding this comment

eopb Sep 14, 2024

Choose a reason for hiding this comment

teamplayer3 commented Sep 15, 2024

eopb commented Sep 22, 2024

teamplayer3 commented Sep 25, 2024 • edited Loading

eopb commented Sep 28, 2024

teamplayer3 commented Sep 28, 2024

teamplayer3 commented Sep 12, 2024 •

edited by eopb

Loading

eopb Sep 14, 2024 •

edited

Loading

teamplayer3 commented Sep 25, 2024 •

edited

Loading