Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add --no-swap flag #193

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

tennox
Copy link

@tennox tennox commented May 17, 2023

See #191 for details

I gave it a shot. Tell me what you think :)

I've also parameterized the tests so the flag is also tested for the same cases.

I did some hyperfine benchmarks to make sure performance isn't severely degraded and it's mostly the same:

$ hyperfine -N -w1 -p 'cp source.txt test.txt' "/home/manu/dev/stuff/sd/target/debug/sd foo bar test.txt" "/home/manu/dev/stuff/sd/target/debug/sd --no-swap foo bar test.txt"

(tried with input sizes: 10B, 1KB, 10MB and containing foo on each line)

@CosmicHorrorDev
Copy link
Collaborator

As a heads up I'll be putting this on the back burner until after the v0.8.0 release. I'm focusing on getting existing functionality fixed up while saving new features for later

@PeterlitsZo
Copy link

LGTM

@PeterlitsZo PeterlitsZo mentioned this pull request Jun 1, 2023
@ThinkChaos
Copy link

(Just a random user's opinion, don't take this with authority)

I think taking the naming from sed would make this more easily discoverable, and is more easily understandable IMO:

-i[SUFFIX], --in-place[=SUFFIX]
    edit files in place (makes backup if SUFFIX supplied)

I'm not a fan of -i[SUFFIX] given -ijkl means -i -j -k -l, but -i=suffix could be nice.

@tennox
Copy link
Author

tennox commented May 10, 2024

Merged latest master & reworked the implementation accordingly (which made it a good bit simpler 👍 )

@tennox
Copy link
Author

tennox commented May 10, 2024

I think taking the naming from sed would make this more easily discoverable, and is more easily understandable IMO:

-i[SUFFIX], --in-place[=SUFFIX]
    edit files in place (makes backup if SUFFIX supplied)

@ThinkChaos thanks, I took (part of) your suggestion, and renamed to --in-place (no short flag / custom suffix implemented).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants