gennaro-tedesco / nvim-possession

📌 the no-nonsense session manager
MIT License
215 stars 7 forks source link

Add new hook: pre_save_hook - runs function before saving. #28

Closed EmreDogann closed 1 year ago

EmreDogann commented 1 year ago

Other Changes

Justification

I was trying to use Scope.nvim, however, I needed to run that plugin's save command just before saving a session. But nvim-posession does not have a pre-save hook, so I wanted to add one with this pull request. With the addition of this new hook, I felt like post_hook would be too confusing, so I renamed it to post_load_hook to make it clearer at what point in the pipeline that hook is called. I also updated the docs for both hooks to reflect the changes.

Please tell me what you think. :-)

gennaro-tedesco commented 1 year ago

Good morning and thank you for your contribution! If I understand correctly the objective of this PR is to add a pre-save hook, namely a custom general function that can be executed before saving the session and that can be used, say, to clean up buffers and views. Am I right?

Some other users made the point in the past to have such a feature, so I guess this is it and it is the right time to include it :)! I will test it throughout the day and then we can merge!

P. S. I would still keep the naming post_hook simply to avoid breaking changes for other users who may be updating the plugin and not notice the change in name. Ideally I would need to open a "BREAKING CHANGES" issue/page that users can subscribe to, in such cases, but since I did not organise it yet I would avoid it for the moment. The pre-save hook could be named save_hook for instance.

EmreDogann commented 1 year ago

a custom general function that can be executed before saving the session and that can be used, say, to clean up buffers and views. Am I right?

You are correct yes. I included a similar example of cleaning up non-visible buffers in the docs for the pre_save_hook.

I would still keep the naming post_hook simply to avoid breaking changes for other users who may be updating the plugin and not notice the change in name.

Ah yes very true, an oversight on my end. To be honest, the name change isn't essential, but if in the future more hooks were to be added it might cause some confusion but even then it might not be a huge deal.

Thank you for taking the time to review this! :-)

gennaro-tedesco commented 1 year ago

The PR looks fine, please re-name the hooks back and we can merge!

but if in the future more hooks were to be added it might cause some confusion

I agree, I will do some naming refactoring afterwards and make a release whose only scope is name deprecation via warning messages

gennaro-tedesco commented 1 year ago

Merged and released in v0.0.11: thank you very much for your contribution!

P. S. Given that now the plugin is relatively stable and used by many, I may drop the 0.0.X semantic numbering and soon release a 0.1.0 to also include better naming of option settings and deprecation warnings.

EmreDogann commented 1 year ago

You're welcome! Happy to help! I gravitated over to your plugin from vim-obsession because it is one of the only session managers that integrates well with fzf-lua.

If you're going to push for a bigger release, then maybe having parity with something like vim-obsession could be nice? I think the only thing missing is the ability to toggle session autosaving. A command like require("nvim-possession").pause() might work which would toggle autosaving for that open nvim instance, then when relaunching nvim it reverts goes back to its setting set in the config's .setup().

With that being said though, I also like how lean this plugin is so this might be too much? I'm not sure, but worth thinking about at the least :-)