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

Overall reorganization #265

Merged
merged 22 commits into from
Nov 18, 2023
Merged

Overall reorganization #265

merged 22 commits into from
Nov 18, 2023

Conversation

nc7s
Copy link
Contributor

@nc7s nc7s commented Nov 2, 2023

This is the refactor I talked about in #238. Sources are unified into a Vec of Mmaps, abstracting away the need for replace_{file,preview}.

@CosmicHorrorDev
Copy link
Collaborator

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)

@nc7s
Copy link
Contributor Author

nc7s commented Nov 2, 2023

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 mmaping stdin), I'm investigating right now.

@CosmicHorrorDev
Copy link
Collaborator

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

@nc7s
Copy link
Contributor Author

nc7s commented Nov 2, 2023

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

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 as STDIN 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.

@CosmicHorrorDev
Copy link
Collaborator

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

@nc7s
Copy link
Contributor Author

nc7s commented Nov 2, 2023

Apart from tests::cli::replace_into_stdout which asserts color codes in stdout, all tests work fine.

"No device" is caused by mmaping 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.

@CosmicHorrorDev
Copy link
Collaborator

Hi! It looks like I caused a lot of conflicts with #266 and #267 😈

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 🤞

@nc7s
Copy link
Contributor Author

nc7s commented Nov 3, 2023

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.

@CosmicHorrorDev
Copy link
Collaborator

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

@nc7s
Copy link
Contributor Author

nc7s commented Nov 3, 2023

--diff would still wait until this goes through. Otherwise the existing code is too spaghetti to stuff new options in.

Copy link
Collaborator

@CosmicHorrorDev CosmicHorrorDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall things look great barring a couple of small things

As for the CI I would just change the expected outputs to get things passing if the only issue is that it's expecting ANSI color codes. I don't want to merge things with CI failing, but we can kick the can on resolving coloring issues for #268

src/input.rs Outdated Show resolved Hide resolved
src/input.rs Outdated Show resolved Hide resolved
src/input.rs Outdated Show resolved Hide resolved
src/input.rs Outdated Show resolved Hide resolved
@nc7s
Copy link
Contributor Author

nc7s commented Nov 8, 2023

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.

@CosmicHorrorDev
Copy link
Collaborator

CosmicHorrorDev commented Nov 8, 2023

Sorry I wasn't really clear. The small things are all the individual comments 😅

@nc7s
Copy link
Contributor Author

nc7s commented Nov 8, 2023

Oh sorry, I'm on my phone so didn't see these.

@nc7s
Copy link
Contributor Author

nc7s commented Nov 9, 2023

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.

@CosmicHorrorDev
Copy link
Collaborator

I'm inclined to agree. Feel free to change that as you see fit

@nc7s
Copy link
Contributor Author

nc7s commented Nov 9, 2023

Failures on Windows only appear in in_place_* tests, my guess is #208 because windows-gnu doesn't have this problem (-gnu was actually cancelled but I hold this point).

@CosmicHorrorDev
Copy link
Collaborator

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

@nc7s
Copy link
Contributor Author

nc7s commented Nov 9, 2023

  • 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 Cows borrowing into them.

Copy link
Collaborator

@CosmicHorrorDev CosmicHorrorDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall things are looking pretty good! Just a few more small things to get through

I believe you can also re-request a review in the future by hitting the refresh icon next to my name under the reviewers section in the top-right if you're ready for me to take a look at things again

src/main.rs Outdated Show resolved Hide resolved
tests/cli.rs Outdated Show resolved Hide resolved
tests/cli.rs Outdated Show resolved Hide resolved
src/input.rs Outdated Show resolved Hide resolved
@CosmicHorrorDev
Copy link
Collaborator

@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!

@nc7s
Copy link
Contributor Author

nc7s commented Nov 15, 2023

I think there are still two active comments that haven't been addressed.

Yeah I replied ;)

@CosmicHorrorDev
Copy link
Collaborator

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 😅

@nc7s

This comment was marked as off-topic.

@CosmicHorrorDev
Copy link
Collaborator

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

Copy link
Contributor Author

@nc7s nc7s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Submitting the review comments. (This is really counterintuitive from my PoV, and from quite a few of others. I don't know why GitHub insists on this.)

src/input.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
tests/cli.rs Outdated Show resolved Hide resolved
src/input.rs Outdated Show resolved Hide resolved
@CosmicHorrorDev
Copy link
Collaborator

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)

Copy link
Collaborator

@CosmicHorrorDev CosmicHorrorDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks great! Thanks for sticking through the unexpectedly long review :)

@CosmicHorrorDev
Copy link
Collaborator

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

@CosmicHorrorDev
Copy link
Collaborator

Another option would be to canonicalize the test home path before normalizing I think

@CosmicHorrorDev CosmicHorrorDev merged commit 2d287b9 into chmln:master Nov 18, 2023
9 checks passed
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.

2 participants