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

Revamp from scratch 🎉 #116

Closed
rmagatti opened this issue Feb 19, 2022 · 7 comments
Closed

Revamp from scratch 🎉 #116

rmagatti opened this issue Feb 19, 2022 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@rmagatti
Copy link
Owner

Motivation

This was my first Neovim plugin. It works well but I am not happy with how the code looks and feels to work on.

Proposal

I want to revamp the design of auto-session towards a few goals, here is a title and a description for each goal:

  1. Cleaner code;
    Some of the code is just hard to read, either because I wasn't aware of the patterns most lua plugins used at the time I first started this or because I didn't know about builtin Neovim lua api functions.
  2. Make more use of Neovim's builtin lua functions;
    This one is pretty self explanatory, I think the plugin could benefit from just reading more of the standard Neovim api and the available helper functions.
  3. Simplify some of the core pattern replacing code for session names;
    I'm not happy with how unclear the code around pattern replacing is, or with how a directory like igs.nvim will end up with a session called igs.. Mostly a bug but just goes to show how unrobust the current solution for sessioin names is. Needs a rework.
  4. Organize things into more granular functions with very clear purposes;
    Some pieces of auto-session are just clunky to read, need too much focus to go through. This is unnecessary, much could be simplified.
  5. Rethink naming and api usage;
    require('autosession') would be a nicer use. Not changing the plugin name, but the lua module might benefit from a rename.
  6. Focus more on Lua over vimscript, simplifying some of the config option handling.
    There's little point in supporting lua and vimscript config options nowadays for a Neovim focused plugin. The vimscript options are set globally, whilst the Lua options are not, having lua only options makes it easier to have much shorter and likely just clearer config names.

Rollout plan

Instead of making tons of breaking changes to auto-session as I go I'm thinking I'd do this in a few steps instead.

  1. Stop feature work on main branch. Bug fixes are still added to the main branch, but I will not personally work on new features;
  2. Branch v2 from main;
  3. v2 becomes the active branch when feature parity is achieved;
  4. Main branch is marked as legacy and date is set for when v2 will be merged into main. (a few weeks? months out? depends really);
  5. v2 is merged into main.

Rollout complete ✅

Dos and Don'ts

  • I do not know when I will start working on this, asking when it'll be done will not help.
  • Want to participate in the rewrite? Please do! It would be a lot easier to not do this alone :)
  • If you feel like you shouldn't use auto-session before the rewrite, I hope you reconsider. I use this plugin every single day, it is reliable and by far one of my "can't live without" plugins for my workflow. Just needs some love with the power of hindsight, that's all :)

Chat?

Contact me on:

  • matrix @rmagatti:matrix.org (I hang out on the Neovim and Neovim Chat channels)
  • twitter @ronniemagatti
    Or just hit me up here in this GH issue.

K bye 👋

@rmagatti rmagatti added the enhancement New feature or request label Feb 19, 2022
@rmagatti rmagatti self-assigned this Feb 19, 2022
@cseickel
Copy link

Hey @rmagatti, thanks for the great plugin! I've been using sessions for a while and I didn't think I needed another plugin, but I got used to the "auto" part very quickly and I can't live without now.

Make more use of Neovim's builtin lua functions;
This one is pretty self explanatory, I think the plugin could benefit from just reading more of the standard Neovim api and the available helper functions.

I did the same thing! After writing a whole library of helper functions by hand, one day I stumbled upon the lua help file and realized my entire utils module was unnecessary! Glad to hear I am not the only one.

The one thing I want to comment on with this plan is that I would not recommend ever merging v2 to main. I say go ahead with new branch and make it the default in github and the docs when you feel it is at feature parity, but there is no reason to merge back to main. Leave main as the implicit v1 and move on. I would like to see it become the norm for everyone to specify a version # branch in their plugin manager instead just automatically following the default branch. Once that happens, we can move past breaking changes being an issue in plugins.

@rmagatti
Copy link
Owner Author

Hey @cseickel thank you for the kind words!

What you are suggesting is worth considering, the problem then becomes having to specify the branch name in the package manager every time instead of just tracking the latest. I know a lot of plugins tend to try and avoid breaking changes nowadays, and when they do happen they try to clearly state so or even provide warning messages for deprecated config options or functions. I don't think just using version numbers as branch names or release tags for that matter solves this completely, as most people just want to do use { 'rmagatti/auto-session', config = function() require('auto-session').setup{} end} and not have to worry about picking a version.

That said, I myself have been bitten by sudden breaking changes in plugins I rely for my day to day use, and when that happens I agree it can certainly be frustrating.

This feature in packer gives me hope though, I'll at the very least start tagging releases for auto-session going forward so people can take advantage of the new wildcard matching.
wbthomason/packer.nvim#797

@DeadlySquad13
Copy link

The one thing I want to comment on with this plan is that I would not recommend ever merging v2 to main. I say go ahead with new branch and make it the default in github and the docs when you feel it is at feature parity, but there is no reason to merge back to main. Leave main as the implicit v1 and move on. I would like to see it become the norm for everyone to specify a version # branch in their plugin manager instead just automatically following the default branch. Once that happens, we can move past breaking changes being an issue in plugins.

I don't think it's a best way to go. In git there's a tag system for that so no need to leave branches with last versions.

@Syakhisk
Copy link

I'm interested in participating!

@rmagatti
Copy link
Owner Author

That's great @Syakhisk! I already have a branch going that has significant changes so some of the auto session code including merging session-lens functionality into here. I'm expecting to get that merged before starting to work on a new refactoring branch for the purpose of making the code overall simpler. In the meantime digging and trying to get familiar with the current code would be good if you'd like to help out with the refactor/revamp 😊

@cameronr
Copy link
Collaborator

@rmagatti Do you think this one could be closed now?

@rmagatti
Copy link
Owner Author

Yeah I believe we're already moving in the right direction we can close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants