feat: replace current match #14

stefanwatt · 2024-07-20T11:09:41Z

Checklist

Used only camelCase variable names.
If functionality is added or modified, also made respective changes to the
README.md (the .txt file is auto-generated and does not need to be modified).

This is just a rough first draft. There's TODOs and bugs still, but I would like to get some feedback at this point to make sure I'm going in a direction that aligns with the project. I tried my best to stick with your naming scheme etc..
I was itching to do some more refactoring, but I want to be respectful to your project. Maybe we can discuss that at some point.

I have not yet adapted the readme, because I wanted to make sure the suggested changes are ok with you.

added functionality:

keep list of matches in state
keep selected match in state
add keymaps for cycling through matches
adapt keymaps for confirmation -> confirm/confirmAll
updated cursor position in targetWin and re-center if necessary
confirm will only replace the selected match

known issues:

seems like something went wrong when solving conflicts
replacement preview shows an extra character that should not be shown
replacement preview is sluggish when there's a lot of matches

… match changes - add keymaps for moving between matches

chrisgrieser · 2024-07-20T11:55:17Z

Hmm, yes, this appears be a larger PR than I expected…

I don't mind refactorings, but they should be a separate PR for clarity reasons. I think it might sense to split up this feature into separate PRs maybe? Like one big PR for all the refactoring you need in preparation for the feature, and then another PR for the feature itself. That way, you can get changes already in, and it would also be easier to review the PR

Another way to split this up to smaller sizes is to disregard ranges at first, and only implement "replace current match" for substitutions on the whole file. And add in ranges later – I assume ranges are one of the things that complicate this feature quite a bit.

stefanwatt · 2024-07-20T13:23:10Z

Perhaps that wasn't clear, but I did not do any optional refactoring, yet.

Like one big PR for all the refactoring you need in preparation for the feature, and then another PR for the feature itself

Sorry but I have no idea what you mean by this or how that could be done. It sounds to me like you are mainly perturbed by the amount of changes? I can't think of a way to split this that would make any sense.

Do you have a way of splitting this in mind?

stefanwatt · 2024-07-20T14:23:26Z

I mean we could probably do the changes to how matches are stored and then used later in a separate PR,
but that's like 95% of this PR.
Then the PR for this feature would be very small, but I'm not convinced that will improve clarity.

chrisgrieser · 2024-07-20T14:32:04Z

So, the current PR changes a total of ~600 lines. Considering that the whole repo is only barely larger, this is unreasonably large.

Looking into it, it appears that the PR has various types of changes:

cosmetic ones (like fixing the casing of classes)
Formatting ones, as it appears you are using something with with a shorter line length. This alone is responsible for half the diff.
Refactorings that prepare the new feature, such as restructured settings or turning local functions into exported functions
The feature itself
Leftover log statements

1. I don't mind, but it should be it's own PR
1. Should be fixed on your end by configuring your formatter properly, it creates so much noise the PR is hard to review
3 and 4 are the actual feature, which as far as I can see, could be split up to make the changes easier to follow.
1. Should be removed

To distinguish 3 and 4, you could think about how far you can make "prepratory" changes without actually adding in the new feature.

Also, your PR is based on an outdated version of the repo, which results in stuff like the regex101 config missing in your fork. You should sync up your fork.

Like all these changes are not sufficient for the PR to be merged, but only for the PR to be in a state where it can be properly reviewed.

stefanwatt · 2024-07-20T16:02:30Z

I understand this needs to be cleaned up and is not ready for review. Like I said, I wanted to synch with you on the direction before I continue. Sorry if this is not standard procedure.

As far as I'm aware I changed the casing in one instance which seemed to clearly be a typo. You capitalized class names everywhere else. I'm not creating a separate PR for a typo.
I use stylua with default settings. Seems that print width is 120 chars by default. That seems pretty generous already. What do you use for formatting? Can we somehow synch our settings?
I ran the formatter again on all the files and that seems to have reduced the number of changes quite a bit.
and 4. I can certainly try to split this up some, but I also don't want to put in work for that unless I know where I'm going. Perhaps you could give me some directions here. What constitutes a preparatory change vs a feature change? When do you find it appropriate to split something into a new PR vs when should it remain together?
^
Totally fair. Of course that needs to be removed before it can be merged. I'm still planning on doing development and I want to continue to use the debug print statements.
I have now removed them for this PR tho and will stash/pop them as needed.

chrisgrieser

Just some minor things I noticed while going through the code

chrisgrieser · 2024-07-21T10:06:27Z

lua/rip-substitute/rg-operations.lua

@@ -1,12 +1,33 @@
 local M = {}
+local config = require("rip-substitute.config").config


config should not be called at the top of the file, since it's then only set once. This prevents the user from running .setup again to change a config. Thus, config should always be set dynamically inside a function

I was wondering about that. For some reason I feel kinda dirty requiring it on every execution of a function.
Is there any drawback to the way you're suggesting?

No, since lua caches it anyway, there is no real drawback. Yeah, it feels a bit weird, since in most other languages, imports should always be placed at the top of the file. But in lua, it's often the better practice to require inside functions for various reasons (lazy evaluation being the other important one). Bit counterintuitive, but you get used to it.

Ok great. That's an obvious choice then.

chrisgrieser · 2024-07-21T10:06:42Z

nvim-float.log

leftover file

lua/rip-substitute/utils.lua

lua/rip-substitute/rg-operations.lua

lua/rip-substitute/popup-win.lua

lua/rip-substitute/matches.lua

chrisgrieser · 2024-07-21T10:51:20Z

Like I said, I wanted to synch with you on the direction before I continue. Sorry if this is not standard procedure.

tbh, I am also not really familiar what the "standard procedure" is. Nonetheless, even just giving some feedback on the ongoing direction is hard, if there is so much noise in the PR, so the cleanup would be needed regardless

As far as I'm aware I changed the casing in one instance which seemed to clearly be a typo. You capitalized class names everywhere else. I'm not creating a separate PR for a typo.

Yes, and due to 600+ lines of diff, it was not easy to tell whether this is a one-time occurrence or something that happens multiple times. ;)

I use stylua with default settings. Seems that print width is 120 chars by default. That seems pretty generous already. What do you use for formatting? Can we somehow synch our settings?
I ran the formatter again on all the files and that seems to have reduced the number of changes quite a bit.

Not sure what exactly you did, but before, the diff had a lot of cases where it split up one line into multiple lines, indicating that some formatter with lower line length was used. Anyway, I use stylua with the config provided in the repo. If you simply run stylua in the repo root, it should by default pick up the config by itself. The diff looks okay now.

Sorry, if I am came off as nitpicky or something, imho keeping noise to a minimum is essential in large PRs, as it's otherwise hard to assess what actual changes have been made. And even if this is a draft PR and therefore still WIP, a large PR with a lot of noise makes it harder to understand what's going on and thus also hard to even give intermediary feedback. In hindsight, most of the issue was due to the formatting issues, but that 1. was less of an issue was in itself hard to tell with so much noise going on there.

and 4. I can certainly try to split this up some, but I also don't want to put in work for that unless I know where I'm going. Perhaps you could give me some directions here. What constitutes a preparatory change vs a feature change? When do you find it appropriate to split something into a new PR vs when should it remain together?

Well, you do a lot of refactorings which make sense, but can be split off into its own PR. And keeping PRs as small is possible is usually desirable (better readability, thus easier reviewability, also cleaner git history).

you do some function renaming, such as executeSubstitution to substituteAll, which make sense, but are these are really only refactorings.
the casing change mentioned in 1 could also go here.
many of the changes in the incremental-preview function of rg-operations.lua seem to be refactorings for making it easier for your new rg-matches module to hook in, e.g. turning parts of the incremental preview function into sub-functions. These refactorings can be made independently of adding your feature.

By moving these into a separate PR, the remaining changes in rg-operations.lua should only be those where your match-logic is inserted, thus making it more transparent how your new feature interacts with the existing plugin code. (Right now, the diff of rg-operations is hard to review, since a lot of it is just code moving around.)

I'm still planning on doing development and I want to continue to use the debug print statements.
I have now removed them for this PR tho and will stash/pop them as needed.

Nice, that's actually a good use case for git stash I dind't know about.

chrisgrieser · 2024-07-21T11:03:52Z

regarding the remaining issues:

seems like something went wrong when solving conflicts

I think this is not the case anymore with your latest changes.

replacement preview shows an extra character that should not be shown

That's probably just an off-by-one issue somewhere since some nvim functions expect columns are zero-based and some expect them to be one-based.

replacement preview is sluggish when there's a lot of matches

Currently (without your PR), rg is run exactly twice on every text change, once to get the matches, and another time to get the replacements. I think the performance issues come from the rg-match module running rg as well. I think it might make sense to cache the rg results in state, and have the incremental preview as well as rg-match access that cache? That would reduce the number of times rg needs to be run.

regarding the general direction:

Will have an in-depth look once the remaining issues are resolved, but I think the general direction is otherwise fine. Separating your feature into mostly a new module makes sense

chrisgrieser

a bunch of more comments, some minor things I noticed, but also some more important stuff like on the keymaps.

chrisgrieser · 2024-07-21T11:25:01Z

lua/rip-substitute/config.lua

@@ -20,14 +20,23 @@ local defaultConfig = {
 	keymaps = {
 		-- normal & visual mode
 		confirm = "<CR>",
+		confirmAll = "<S-CR>",


for the sake of not introducing a breaking change for existing users, the current keymaps behavior should not be changed.

confirm should be used to confirm all like it's doing now, and your feature should introduce a confirmSingle which uses a new keymaps <S-CR>. Yes, it's not ergonomic, but otherwise your PR creates a breaking change which will irritate users.

chrisgrieser · 2024-07-21T11:26:03Z

lua/rip-substitute/config.lua

-		local msg =
-			"`incrementalPreview.hlGroups.{name}` is deprecated, use `incrementalPreview.matchHlGroup` instead."
-		notify(msg, "warn")
+		-- local msg =


as annoying as it is, I'm afraid we will need a deprecation notice for the now basically reverted setting…

just to make sure I understand correctly:
the new deprecation notice would be:
"incrementalPreview.matchHlGroup is deprecated, use incrementalPreview.hlGroups.{name} instead."
?

lua/rip-substitute/config.lua

chrisgrieser · 2024-07-21T11:32:23Z

lua/rip-substitute/matches.lua

+	end
+end
+
+function M.selectPrevMatch()


selectPrevMatch and selectNextMatch could be merged into one function taking the direction as a parameter

are you ok with keeping shorthand functions like this:

---@param direction "next" | "prev" local function cycleMatches(direction) ... function M.selectNextMatch() cycleMatches("next") end function M.selectPrevMatch() cycleMatches("prev") end

why even the wrapper functions? You can just assign cycleMatches to the keymap definitions for the popup win?

perhaps I'm ignorant here...
As far as I'm aware the only way to do that would be with an inline function since vim.keymap.set doesn't have a mechanism to pass extra args to the callback function.
So If i have to write function()...end, I might as well do it like this instead of the inline function.
That was my thinking process anyway

lua/rip-substitute/utils.lua

chrisgrieser · 2024-07-21T11:43:55Z

lua/rip-substitute/matches.lua

+		)
+	end
+	state.selectedMatch = state.matches[newSelectedMatchIndex]
+	--TODO: this will probably clear too many highlights


yes, an issue will be multiple matches in one line, for instance. Not totally sure what would be the best solution here though

lua/rip-substitute/rg-operations.lua

chrisgrieser · 2024-07-21T11:47:26Z

lua/rip-substitute/rg-operations.lua

+---@param selected boolean
+---@param targetBuf number
+---@param incPreviewNs number
+function M.highlightReplacement(match, selected, targetBuf, incPreviewNs)


targetBuf and incPreviewNs do not need to be passed, since they can be accessed via state.

chrisgrieser · 2024-07-21T11:47:59Z

lua/rip-substitute/rg-operations.lua

+---@param targetBuf number
+---@param incPreviewNs number


targetBuf and incPreviewNs do not need to be passed, since they can be accessed via state.

lua/rip-substitute/popup-win.lua

stefanwatt · 2024-07-21T13:01:50Z

Wow. That's a lot to take in 😄

I think I have a better idea of what you are looking for now.

I agree with all the remarks and will try to adapt the PR accordingly.

Sorry, if I am came off as nitpicky or something

All good 👍 To be honest I was getting a bit into my feelings because of some of the critique, but that might just be me. I'm a sucker for words of affirmation 😄
I know it's also a lot of work for you to maintain your projects.

imho keeping noise to a minimum is essential in large PRs

completely fair. I guess I got too excited and pushed too early.

- utils.map -> vim.tbl_map - no more pcall - renamed some functions/options to have less breaking changes - remove useless --vimgrep - use vim.o.scrolloff instead of magic number for padding - some minor refactoring

stefanwatt · 2024-07-21T20:03:00Z

Ok I think I took all your remarks into account and made adaptations accordingly.
Now I'll try to separate out the preparatory changes.

Stefan Watt and others added 8 commits June 28, 2024 23:04

can get a list of matches on TextChanged

a425d1a

- get closest match after cursor works

4e89d08

fixed bug with rg replacement

be6b860

fixed match range by adding --only-matching

39e3544

- change highlight based on selected match and rerender when selected…

3cd342a

… match changes - add keymaps for moving between matches

-replace current match sorta works

76467b5

centering cursor works well now

25b7f50

Merge branch 'main' into feature/replace-current-match

ecce42d

stefanwatt changed the title ~~Feature/replace current match~~ feat replace current match Jul 20, 2024

stefanwatt changed the title ~~feat replace current match~~ feat: replace current match Jul 20, 2024

- remove print statements

c3e2490

chrisgrieser reviewed Jul 21, 2024

View reviewed changes

chrisgrieser requested changes Jul 21, 2024

View reviewed changes

Stefan Watt added 2 commits July 21, 2024 21:11

remove utils.map in favor of vim.tbl_map

9249fee

- always re-execute require of config and state

38d94cf

- utils.map -> vim.tbl_map - no more pcall - renamed some functions/options to have less breaking changes - remove useless --vimgrep - use vim.o.scrolloff instead of magic number for padding - some minor refactoring

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace current match #14

feat: replace current match #14

stefanwatt commented Jul 20, 2024 •

edited

Loading

chrisgrieser commented Jul 20, 2024 •

edited

Loading

stefanwatt commented Jul 20, 2024 •

edited

Loading

stefanwatt commented Jul 20, 2024

chrisgrieser commented Jul 20, 2024 •

edited

Loading

stefanwatt commented Jul 20, 2024 •

edited

Loading

chrisgrieser left a comment

chrisgrieser Jul 21, 2024

stefanwatt Jul 21, 2024

chrisgrieser Jul 21, 2024 •

edited

Loading

stefanwatt Jul 21, 2024

chrisgrieser Jul 21, 2024

chrisgrieser commented Jul 21, 2024 •

edited

Loading

chrisgrieser commented Jul 21, 2024 •

edited

Loading

chrisgrieser left a comment

chrisgrieser Jul 21, 2024 •

edited

Loading

chrisgrieser Jul 21, 2024

stefanwatt Jul 21, 2024 •

edited

Loading

chrisgrieser Jul 21, 2024

chrisgrieser Jul 21, 2024

stefanwatt Jul 21, 2024

chrisgrieser Jul 21, 2024

stefanwatt Jul 21, 2024 •

edited

Loading

chrisgrieser Jul 21, 2024

chrisgrieser Jul 21, 2024

chrisgrieser Jul 21, 2024

stefanwatt commented Jul 21, 2024

stefanwatt commented Jul 21, 2024

		@@ -1,12 +1,33 @@
		local M = {}
		local config = require("rip-substitute.config").config

feat: replace current match #14

Are you sure you want to change the base?

feat: replace current match #14

Conversation

stefanwatt commented Jul 20, 2024 • edited Loading

Checklist

chrisgrieser commented Jul 20, 2024 • edited Loading

stefanwatt commented Jul 20, 2024 • edited Loading

stefanwatt commented Jul 20, 2024

chrisgrieser commented Jul 20, 2024 • edited Loading

stefanwatt commented Jul 20, 2024 • edited Loading

chrisgrieser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisgrieser Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisgrieser commented Jul 21, 2024 • edited Loading

chrisgrieser commented Jul 21, 2024 • edited Loading

chrisgrieser left a comment

Choose a reason for hiding this comment

chrisgrieser Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanwatt Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanwatt Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanwatt commented Jul 21, 2024

stefanwatt commented Jul 21, 2024

stefanwatt commented Jul 20, 2024 •

edited

Loading

chrisgrieser commented Jul 20, 2024 •

edited

Loading

stefanwatt commented Jul 20, 2024 •

edited

Loading

chrisgrieser commented Jul 20, 2024 •

edited

Loading

stefanwatt commented Jul 20, 2024 •

edited

Loading

chrisgrieser Jul 21, 2024 •

edited

Loading

chrisgrieser commented Jul 21, 2024 •

edited

Loading

chrisgrieser commented Jul 21, 2024 •

edited

Loading

chrisgrieser Jul 21, 2024 •

edited

Loading

stefanwatt Jul 21, 2024 •

edited

Loading

stefanwatt Jul 21, 2024 •

edited

Loading