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

Fix longstanding bug in replace #32

Merged
merged 1 commit into from
Feb 10, 2022
Merged

Fix longstanding bug in replace #32

merged 1 commit into from
Feb 10, 2022

Conversation

telemachus
Copy link
Collaborator

For several years, replace has not worked in normal mode. This commit
fixes that bug. In addition, I've updated the tests and README a bit.

At this point, all existing tests are passing. There's a clean slate for
new changes or CI.

(The bug was added in #19. In the end, the fix was trivial, but it took
me too long to see it. I'd like to set up CI now, so I'll start in on that
branch after this is merged. I'd also like to update the test files to be
more sandboxed from the user's larger vim environment.)

For several years, replace has not worked in normal mode. This commit
fixes that bug. In addition, I've updated the tests and README a bit.

At this point, all existing tests are passing. There's a clean slate for
new changes or CI.
@telemachus telemachus requested a review from alerque February 10, 2022 13:45
@alerque
Copy link
Member

alerque commented Feb 10, 2022

I just setup Vader in CI twice in the last two days, if you like I can probably whip it out easily while it's fresh in my mind.

@alerque
Copy link
Member

alerque commented Feb 10, 2022

c.f. junegunn/vader.vim#224

@telemachus
Copy link
Collaborator Author

I just setup Vader in CI twice in the last two days, if you like I can probably whip it out easily while it's fresh in my mind.

That would be terrific! If it's okay with you, I may add/remove/rework
the tests after that, but I'm not familiar with GitHub's CI at all. If you
do the initial setup, I'll study that to learn more about CI here. Then,
I should be able to help more with that going forward.

(Btw, I don't seem to be authorized to merge the pull request. Please
go ahead and do that if it looks good to you. Thanks!)

@alerque
Copy link
Member

alerque commented Feb 10, 2022

That would be terrific! If it's okay with you, I may add/remove/rework the tests after that, but I'm not familiar with GitHub's CI at all.` If you do the initial setup, I'll study that to learn more about CI here. Then, I should be able to help more with that going forward.

Sure, should be just a few minutes. For now I won't setup a way to run it locally (you seem to have that figured out anyway) but will add CI checks to make sure they are run remotely before future merges.

(Btw, I don't seem to be authorized to merge the pull request. Please go ahead and do that if it looks good to you. Thanks!)

Can you try again? I just fiddled with permissions, it may not have been setup right. It would be nice to get that straightened out.

@telemachus telemachus merged commit 164629d into master Feb 10, 2022
@telemachus
Copy link
Collaborator Author

Can you try again? I just fiddled with permissions, it may not have been setup right. It would be nice to get that straightened out.

Yes, I can now merge (and I have). Thanks.

@alerque alerque deleted the fix-replace branch February 10, 2022 14:55
@telemachus
Copy link
Collaborator Author

In the future, I should probably handle the merge commit better.
I apologize: I'm not used to dealing with merges through GitHub
as opposed to the command line.

@alerque
Copy link
Member

alerque commented Feb 10, 2022

For the record, some tests were failing until I merged this work into my CI branch, now they all pass. So you did something right ;-)

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