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

feat: add defer_save, cancel_save, immediate_save trigger options #11

Merged
merged 6 commits into from
Mar 28, 2023

Conversation

primeapple
Copy link
Collaborator

@primeapple primeapple commented Mar 22, 2023

This fixes #8

@primeapple primeapple force-pushed the feature/8-improved-saving-method branch from e3e3800 to 8cf0bf0 Compare March 22, 2023 08:46
@primeapple
Copy link
Collaborator Author

Just from the codestructure I'm happy now.
I'll try using it from next week on and see if there are any bugs.

@okuuva feel free to have a look over it.

I also added some TODOs that might be good for later improvements.

@okuuva
Copy link
Owner

okuuva commented Mar 24, 2023

Excellent work @primeapple! I'll take it into daily use as well to spot any potential problems but LGTM already :)

@okuuva
Copy link
Owner

okuuva commented Mar 24, 2023

And I do agree with the TODOs. I wouldn't maybe drop those in this MR just yet just to keep things as atomic as possible. And I need to comb through the code a few more times since I've also wondered what's the point with those seemingly unused things. Probably remnants from the rewrite Pocco81 did in July 2022 but need to triple check.

@primeapple
Copy link
Collaborator Author

primeapple commented Mar 26, 2023

And I do agree with the TODOs. I wouldn't maybe drop those in this MR just yet just to keep things as atomic as possible.

I'll remove the todos and open another PR afterwards to add them, maybe even removing the potential unused code.

Thanks for the review :)

@primeapple
Copy link
Collaborator Author

I have some trouble because it doesn't seem to be possible to set timers to buffer variables, see here: https://www.reddit.com/r/neovim/comments/123y048/cannot_convert_userdata_when_trying_to_set_a/

I might just use the global_vars variable after all...

@primeapple
Copy link
Collaborator Author

Ok. I open this as "Ready for review". It works quite well on my machine now, you are save to try it out @okuuva :)

@primeapple primeapple marked this pull request as ready for review March 27, 2023 21:04
@primeapple primeapple requested a review from okuuva March 27, 2023 21:04
Copy link
Owner

@okuuva okuuva left a comment

Choose a reason for hiding this comment

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

Haven't had time to test this thoroughly since I was away for the weekend and been sick since I got back. But couldn't find any obvious problems with the short time I had to test it, the code looks good to me and I trust you've been testing it yourself during the development much more rigorously than I have. Exceptional job!

@okuuva okuuva changed the title Introduced defer_save, cancel_save, immediate_save trigger options feat: add defer_save, cancel_save, immediate_save trigger options Mar 28, 2023
@okuuva okuuva merged commit a5e3955 into okuuva:main Mar 28, 2023
@primeapple
Copy link
Collaborator Author

Ok, thank you!
Let's observe how it behaves in the future ;-)

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.

Save only after sequence of actions finished, not after each action
2 participants