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

Add an opt-in feature that saves a session per git branch (if you are in a git repo). #121

Merged
merged 1 commit into from
Apr 12, 2022
Merged

Add an opt-in feature that saves a session per git branch (if you are in a git repo). #121

merged 1 commit into from
Apr 12, 2022

Conversation

WhoIsSethDaniel
Copy link
Contributor

No description provided.

@WhoIsSethDaniel
Copy link
Contributor Author

bare-bones effort to get session per git branch.

-- get the current git branch name, if any, and only if configured to do so
local function get_branch_name()
if AutoSession.conf.auto_session_use_git_branch then
local out = vim.fn.systemlist('git rev-parse --abbrev-ref HEAD')
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a drive by comment but this function systemlist is synchronous and will block nvim till it completes so if this is run at startup it will slow users startups down. An alternative is to use jobstart and pass a callback to receive the result, but then the whole outer function will have to adapt to use callbacks as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm aware. Redoing / adapting auto-session to use jobs isn't going to happen. At least not by me. Sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

@WhoIsSethDaniel no worries, fully a drive by comment, just pointing it out in case you weren't aware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may be an idea for #116. I wonder if the rewrite would benefit from using plenary. It has excellent job support.

Copy link
Owner

@rmagatti rmagatti Mar 17, 2022

Choose a reason for hiding this comment

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

Good input, this is something I've thought about a bit, I'm just not sure if the core restoring and saving a session behaviour could actually be offloaded to a job. 🤔 As far as this git functionality, it's a good use-case for a job I'd say.

I also agree @WhoIsSethDaniel that job support is a feature better suited for the rewrite. I'll keep this in mind when working on it.

Thanks for chiming in @akinsho !

@rmagatti
Copy link
Owner

I'll try to test this out sometime this weekend, will merge if it looks good then!

@rmagatti rmagatti self-requested a review April 4, 2022 21:02
Copy link
Owner

@rmagatti rmagatti left a comment

Choose a reason for hiding this comment

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

Took me a while to get to this but this works well 👍

@rmagatti
Copy link
Owner

rmagatti commented Apr 4, 2022

@WhoIsSethDaniel I'll try and add some documentation about this feature in the readme before merging, or you can do it if you happen to have a free minute. Either way, nice work, thanks for the contribution! 🎉

@WhoIsSethDaniel
Copy link
Contributor Author

@WhoIsSethDaniel I'll try and add some documentation about this feature in the readme before merging, or you can do it if you happen to have a free minute. Either way, nice work, thanks for the contribution! tada

I may have time tonight. Thanks for the review.

@WhoIsSethDaniel
Copy link
Contributor Author

I added the new config element to README.md

@rmagatti rmagatti merged commit e840b84 into rmagatti:main Apr 12, 2022
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.

3 participants