Closed nc7s closed 9 months ago
Don't have time to dig in much, but from a quick peek I wouldn't expect coloring for the replacements to work right without trying tty detection, and it looks like the code doesn't limit the number of replacements in general now when passing the -n <limit>
flag (this is also already a bug when passing --preview
where -n
isn't respected. I'm working on fixing that today, and those changes should make it easier to fix in this PR as well)
Rather bizarre to see the test hard-code color codes into asserts. tty detection and automatic coloring is another PR I wanted to throw in, after this one gets through (or not, both fine).
Limiting number of replacements works by Regex::replacen(_, limit: self.replacements, _)
, which, after this PR, now works across all situations AFAICT.
My changes does introduce a No such device
error (supposedly caused by mmap
ing stdin
), I'm investigating right now.
Limiting number of replacements works by
Regex::replacen(_, limit: self.replacements, _)
, which, after this PR, now works across all situations AFAICT.
The part that I don't think will work is handling coloring the replacements when the output is a tty or when the user passes --preview
, or at least I don't know of a way to get it to work without rolling our own replacen()
which was my plan
Re: no device
My guess is that it happens when reading from stdin
while not using --preview
. It looks like it will try to create a file within the same directory as STDIN
which will obviously cause problems
Also thanks for taking up the initiative to fix things up. @dev-ardi and I were conveniently talking about the overall architecture yesterday
Limiting number of replacements works by
Regex::replacen(_, limit: self.replacements, _)
, which, after this PR, now works across all situations AFAICT.The part that I don't think will work is handling coloring the replacements when the output is a tty or when the user passes
--preview
, or at least I don't know of a way to get it to work without rolling our ownreplacen()
which was my plan
This one can be solved with Regex::split()
combined with Iter::take(n)
. I actually kept the split part in an earlier draft but it's lost with a pull, then overlooked.
Re: no device
My guess is that it happens when reading from
stdin
while not using--preview
. It looks like it will try to create a file within the same directory asSTDIN
which will obviously cause problems
Seems so. On it.
Also thanks for taking up the initiative to fix things up. @dev-ardi and I were conveniently talking about the overall architecture yesterday
Despite two weeks after my "ambitious" words ;) I'm the one pushing for 1.0 after all.
Despite two weeks after my "ambitious" words ;) I'm the one pushing for 1.0 after all.
Any help is very much so appreciated! I'm hoping to have the beta published today after getting that -n
with --preview
bug fixed
Apart from tests::cli::replace_into_stdout
which asserts color codes in stdout, all tests work fine.
"No device" is caused by mmap
ing stdin
, fixed with a full read and copy into a mmap. Or rather worked around, since large input could result in two copies in memory.
For the STDIN
fake path, added a check so --preview
and stdin are equal in terms of output target selection.
Hi! It looks like I caused a lot of conflicts with #266 and #267 :smiling_imp:
Fortunately master
should actually be a lot more similar to this PR now that the bug where --preview
would ignore -n
is fixed
Rather bizarre to see the test hard-code color codes into asserts. tty detection and automatic coloring is another PR I wanted to throw in, after this one gets through (or not, both fine).
100% agree. I think it expresses how spotty our support for determining when color should be used really is. I've gone ahead and opened #268 as a bit of a wishlist for what I'd like to see supported
Also (I doubt this is a surprise at this point), but I won't be getting around to reviewing this before v1.0 is released as a heads up. That should mean less than a week at this point :crossed_fingers:
Hi! It looks like I caused a lot of conflicts with #266 and #267 😈
Just a rebase away.
Fortunately
master
should actually be a lot more similar to this PR now that the bug where--preview
would ignore-n
is fixed
Great! Saw the changes (then removed replace_preview()
again 😈)
Rather bizarre to see the test hard-code color codes into asserts. tty detection and automatic coloring is another PR I wanted to throw in, after this one gets through (or not, both fine).
100% agree. I think it expresses how spotty our support for determining when color should be used really is. I've gone ahead and opened #268 as a bit of a wishlist for what I'd like to see supported
Nice, I'll put some known things there.
Also (I doubt this is a surprise at this point), but I won't be getting around to reviewing this before v1.0 is released as a heads up. That should mean less than a week at this point 🤞
Sure, it's mostly an internal cleanup. Do you think --diff
can make it in though? If so I'll make up over there.
Also (I doubt this is a surprise at this point), but I won't be getting around to reviewing this before v1.0 is released as a heads up. That should mean less than a week at this point 🤞
Sure, it's mostly an internal cleanup. Do you think
--diff
can make it in though? If so I'll make up over there.
I was planning on leaving that till v1.1, but if you want to see it in the next release then I should have enough time to look over all of it and try to get it merged
--diff
would still wait until this goes through. Otherwise the existing code is too spaghetti to stuff new options in.
What are the small things? I'll try to fix them.
Regarding color codes, I intend to nudge them there too. Maybe disable related tests first.
Sorry I wasn't really clear. The small things are all the individual comments 😅
Oh sorry, I'm on my phone so didn't see these.
More than that, I'm also kinda confused by struct App
being in src/input.rs
, and the existence of App
itself. It merely holds a Source
(after this PR a Vec<Source>
) and a Replacer
. It does hold the CLI logic, but I'd argue that should be put in src/main.rs
.
I'm inclined to agree. Feel free to change that as you see fit
Failures on Windows only appear in in_place_*
tests, my guess is https://github.com/chmln/sd/pull/208 because windows-gnu doesn't have this problem (-gnu was actually cancelled but I hold this point).
My guess is on some platform specific quirks with filesystem/mmap stuff in write_with_temp
. I don't see anything from looking at it, but there are some subtle changes to how things are handled
I can setup a windows dev env and work on trying to debug this tomorrow. I'd also like to have some kind of logging added which can help with debugging these issues in the future
I have a Windows box at hand, can test it. Logging is probably good to have.
in_place_following_symlink
shouldn't be run on Windows, symlinks there are privileged.in_place_with_empty_result_file
is seemingly caused by Windows needing no escape of \
on the command line, while Rust still escaping it.in_place
and limit_replacements_file
seem to be tempfile failing to persist. My guess is Windows refusing to write to a file still open, even if opened read only. Current implementation holds all mmaps in memory, with result Cow
s borrowing into them.@nc7s Sorry for the delay on the re-review. I think there are still two active comments that haven't been addressed. Other than that all of the changes look good to me!
I think there are still two active comments that haven't been addressed.
Yeah I replied ;)
I think either you forgot to hit something to send the replies, or I'm just missing something because I'm only seeing my initial comment on both the open ones 😅
@CosmicHorrorDev can you see this?
Ah no. The 'Pending' indicates that you need to submit the review or something like that for everyone else to see. It depends on which tab of the PR you're using, but that's how the code section is I believe
Submitting the review comments. (This is really counterintuitive from my PoV.)
Yeah I totally get that. For just replying to a message you'll want the send single message button. If you want to do a coordinated set of replies you want a full review (yes it's incredibly non-obvious)
Whelp spoke a bit too soon. It looks like the path is different on apple. I'm fine with those tests being switched to just Linux if you are too. I was a little worried that the error message would differ on other unix platforms, but can't test locally
Another option would be to canonicalize the test home path before normalizing I think
This is the refactor I talked about in https://github.com/chmln/sd/pull/238. Sources are unified into a
Vec
ofMmap
s, abstracting away the need forreplace_{file,preview}
.