search

uutils / findutils

Rust implementation of findutils
MIT License
300 stars 37 forks source link

xargs: do not merge extra args before replacing #363

Closed YDX-2147483647 closed 3 months ago

YDX-2147483647 commented 5 months ago

Resolves #362

# Before
$ printf 'a    bc\ndef\ng' | xargs -I _ echo '[_]'
[a bc def g]

# After
$ printf 'a    bc\ndef\ng' | cargo run --bin xargs -- -I _ echo '[_]'
[a    bc]
[def]
[g]
codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 91.78082% with 6 lines in your changes missing coverage. Please review.

Project coverage is 60.48%. Comparing base (a2e350c) to head (9bc80cd).

Files Patch % Lines
src/xargs/mod.rs 86.66% 3 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #363 +/- ## ========================================== + Coverage 60.03% 60.48% +0.44% ========================================== Files 32 32 Lines 4021 4069 +48 Branches 891 895 +4 ========================================== + Hits 2414 2461 +47 Misses 1254 1254 - Partials 353 354 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

YDX-2147483647 commented 5 months ago

Hi! I've fixed the issue in normal cases, but if conflict options are given, things get complicated.

Currently, we throw a warning and take the last option.

https://github.com/uutils/findutils/blob/baa09baf47992a074538ed0e4bb3c4e952f37111/src/xargs/mod.rs#L930-L933

Should I follow this behaviour and change 100+ lines? Or may I use Arg::conflicts_with in clap and throw an error?

(Modified from https://github.com/uutils/findutils/issues/362#issuecomment-2081512700 )

YDX-2147483647 commented 5 months ago

@sylvestre Hello, may I ask your opion on https://github.com/uutils/findutils/pull/363#issuecomment-2081766265? (No need to review codes, just a design question.)

sylvestre commented 4 months ago

@YDX-2147483647 sorry, i missed your question :( you should do the same as the GNU implementation

YDX-2147483647 commented 4 months ago

Never mind and thanks for your answer.

So those 100+ lines is worth changing. I'll update the branch and add more tests when I have time.

YDX-2147483647 commented 4 months ago

Well, I've updated the branch. Could someone approve the CI? I think this PR is ready now.

sylvestre commented 4 months ago

it it ready for review? :) thanks

YDX-2147483647 commented 4 months ago

Yes! It is ready.

sylvestre commented 3 months ago

nice: Warning: Changes from main: PASS +4 / FAIL -4 / ERROR +0 / SKIP +0