Shougo / dein.vim

:zap: Dark powered Vim/Neovim plugin manager
MIT License
3.42k stars 197 forks source link

🚀 Major improvement of UNIX installer script #494

Closed ghost closed 1 year ago

ghost commented 1 year ago

I've refactored and added some functionalities to improve the overall experience while running the UNIX installer script.

Changes

Important notes

Cleanup on error

The script will now automatically delete the cloned Dein.vim repo when the script runs into a major error. This allows the script to run again without showing the Error: Folder already exists. message.

Obs.: I wonder if we could delete the existing cloned repo automatically? This way this Error: Folder already exists. message would probably never appear.

Existing config

Any existing Vim or Neovim config file will be saved inside a backup file with a unique suffix (eg. .pre-dein-vim or -{TIMESTAMP}.pre-dein-vim) in the same directory. The {TIMESTAMP} is the date that the backup was created in the format month-day-year-hour-minutes-seconds, the timestamp will only be added when dealing with multiple backup files.

CLI Arguments

The base path command line argument now supports an quoted path argument (eg. sh ./installer.sh "~/.cache/dein") or an path with trailing slash (eg. sh ./installer.sh ~/.cache/dein/). However, the matching rule has become more shallow/generic to allow quoted paths, which could possible cause some rare edge-cases problems. Since the main-cases works just fine, I still added it.

Other notes

The Git log messages will now be logged, especially from the git fetch command, since it's relevant to at least have an downloading feedback.

The ending messages of the installer script will now have more relevant informations such as documentation, chat, and report link.

ghost commented 1 year ago

What do you think about the script automatically cleaning an old clone on start, if any?

This would allow user to run the script to update their setup without having to manually delete the existing clone of dein.vim.

Example:

Installation of dein.vim with ~/.cache/dein as the base path.

If the user run the script again it would delete the old Shougo/dein.vim folders inside of ~/.cache/dein/repos/github.com/, allowing the script to automatically update it, without removing any other plugin inside of ~/.cache/dein/.

This would be surprisingly simple, and would not break any other existing functionalities.

Shougo commented 1 year ago

Hm....

This would allow user to run the script to update their setup without having to manually delete the existing clone of dein.vim.

I don't like the automatic remove.

Shougo commented 1 year ago

Hm... I may split the installer to another repository. Because I don't use the installer and the script seems complex.

ghost commented 1 year ago

I don't like the automatic remove.

Why? we aren't removing other plugins only the dein.vim cloned repo.

It's likely that an user that is running the script again after installing, might be wanting to update it, and since the auto clean would not be removing other cloned repositories or plugins, there's no problem with it.

Example:

If maybe the user installed a plugin from Shougo meaning it would be inside ~/.cache/dein/repos/github.com/Shougo/ nothing but the old dein.vim folder inside of it would be removed.

What's the problem with this? I'm genuinely curious right now.

ghost commented 1 year ago

Hm... I may split the installer to another repository. Because I don't use the installer and the script seems complex.

Yeah, however, a lot of new users will use it, since it's common for an project like this to have one. I known it can be a lot for those who don't deal with Shellscript daily, but this effort is worth.

ghost commented 1 year ago

If you find it difficult to understand everything, you can let this open for a bit more and let the community leave their thoughts. Don't need to rush.

Shougo commented 1 year ago

Yeah, however, a lot of new users will use it, since it's common for an project like this to have one. I known it can be a lot for those who don't deal with Shellscript daily, but this effort is worth.

I know it may be useful for beginners. But dein.vim is not for beginners. The script has been complex.

If you find it difficult to understand everything, you can let this open for a bit more and let the community leave their thoughts. Don't need to rush.

Yes. I need to time. But users don't comment about the PR...

Shougo commented 1 year ago

If I split the installer, the installation description will be simpler.

https://github.com/Shougo/dein.vim#getting-started

Shougo commented 1 year ago

Why? we aren't removing other plugins only the dein.vim cloned repo.

It's likely that an user that is running the script again after installing, might be wanting to update it, and since the auto clean would not be removing other cloned repositories or plugins, there's no problem with it.

OK. If so, another option --upgrade should be added for it.

ghost commented 1 year ago

OK. If so, another option --upgrade should be added for it.

If we were to add an --upgrade argument, it would be better to do in another PR.

Right now, we could merge this one after reviewing the current changes.

Shougo commented 1 year ago

Well, I will split the installer to another repository. I have heard users voice.

ghost commented 1 year ago

It's my first time seeing someone split an installation script with less than 500 lines of code. But, it's up to you in the end.

And the "big README.md" file can be fixed if the project uses GitHub features more properly. Like using the Wiki pages and Discussions.

I'll be off for some time, since Christmas is approaching 🎄

Happy Xmas! Even if it's too early 😅

Shougo commented 1 year ago

It's my first time seeing someone split an installation script with less than 500 lines of code. But, it's up to you in the end.

Yes. It is my decision. I want to keep the repository simple.

Happy Xmas! Even if it's too early sweat_smile

Thanks.

Shougo commented 1 year ago

https://github.com/Shougo/dein-installer.vim

I have split the installer. Please create new PR for it instead.