-
Notifications
You must be signed in to change notification settings - Fork 0
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(config): add vim config #61
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to the Vim configuration, including the addition of a custom alias for launching Vim with a specific configuration file. It implements the Vim-Plug plugin manager for better plugin management and adds a new entry to the Changes
Sequence DiagramsequenceDiagram
participant User
participant Vim
participant VimPlug
participant Plugins
User->>Vim: Launch with custom config
Vim->>VimPlug: Check plugin manager
alt Plugin manager not installed
VimPlug->>VimPlug: Install self
end
Vim->>VimPlug: Load plugins
VimPlug->>Plugins: Install/Update plugins
Plugins-->>Vim: Configure environment
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (5)
config/vim/autoload/plug.vim (1)
958-1009
: Validate shell environment changes on WindowsIn the function
s:chsh(swap)
, the shell environment is temporarily changed. This approach may fail under certain custom user shells or localized Windows environments (e.g., alternative code pages or unusual&shell
). Consider improving the fallback logic or providing explicit error messages if the expected shell environment isn’t found.config/vim/vimrc (4)
25-36
: Check if auto-install of vim-plug might disrupt user workflowAutomatically installing vim-plug on Vim startup is convenient, but forcing
:PlugInstall
on first run can surprise users. Consider offering an opt-in prompt or a documented environment variable to skip. This helps advanced users who may manage vim-plug via system packages or prefer manual installation.
321-321
: Use or remove the FixWhitespace command consistentlyThe command
FixWhitespace
is handy, but consider hooking it into your development workflow (e.g., pre-commit hooks) or removing it if it’s not part of your typical usage. Stale commands can clutter user interfaces.
493-503
: Refine or unify build commands for Go filesIn
s:build_go_files()
, you differentiate_test.go
vs. regular.go
for building or testing. This is helpful, but you might unify them into a single command or mapping (e.g., “run relevant tests if_test.go
is open, otherwise build”), or let the user select. This might avoid confusion for multi-file contexts.
634-635
: Confirm Python syntax highlighting
let python_highlight_all = 1
might override certain language-specific color schemes or produce conflicting highlights in some themes. If you see inconsistent highlighting, consider toggling this or verifying theme compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/alias
(1 hunks)config/vim/.gitignore
(1 hunks)config/vim/autoload/plug.vim
(1 hunks)config/vim/vimrc
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/vim/.gitignore
🔇 Additional comments (8)
config/vim/autoload/plug.vim (2)
315-323
: Ensure user confirmation logic remains robust
The function s:ask(message, ...)
and its companion s:ask_no_interrupt(...)
handle user confirmation. Given their importance for directory deletion, plugin removal, and other potentially destructive operations, ensure enough user feedback is provided about the repercussions of each action (especially with [y/N/a]
prompts). This helps prevent accidental data loss.
1405-1452
: Handle job execution failures consistently
The s:spawn
function and subsequent callbacks handle job startup and concurrency. If a job fails to start or is misconfigured (e.g., invalid arguments), the code sets job.running = 0
and job.error = 1
. Confirm that dependent processes or commands do not continue in partial states when a job fails. Consider centralizing error handling to prevent partial updates.
config/alias (1)
12-12
: Great addition for custom Vim configuration
Using alias vim='vim -u "$XDG_CONFIG_HOME/vim/vimrc"'
ensures that Vim always starts with the desired config, without polluting the user’s standard configuration. This looks good for isolating plugin settings and caches under $XDG_CONFIG_HOME
.
config/vim/vimrc (5)
68-72
: Ensure fallback to 'make' is valid
You set let g:make='gmake'
but then override it with 'make'
if exists('make')
. Check if that logic is reversed: typically, you’d prefer gmake
only if a system doesn’t have standard make
. Double-check to guarantee building plugins like vimproc won’t fail on systems lacking gmake
.
185-190
: Session management settings appear consistent
The session directory and related options (session_autoload
, session_autosave
, etc.) are neatly consolidated. Just ensure any external scripts referencing sessions are updated to read from ~/.local/state/vim/session
, avoiding confusion with default Vim session directories.
557-559
: Review ALE linters array creation syntax
The extension g:ale_linters
is extended for Go with golint
and go vet
. Ensure each linter name is recognized by ALE. Typos or mismatch with ALE’s actual linter ID can cause silent failures.
631-632
: Convenient environment detection in statusline
Enabling airline#extensions#virtualenv#enabled
helps identify which virtual environment is active. Confirm that your environment variables (e.g., VIRTUAL_ENV
) are consistently set across shells, especially on macOS or Windows, for accurate detection.
644-647
: Vue plugin compatibility
Disabling pre-processors with vue_disable_pre_processors=1
and enabling g:vim_vue_plugin_load_full_syntax
can be beneficial for simpler projects but watch out if using advanced Vue SFC syntax with custom lang
attributes. Some features might not parse fully.
if isdirectory(a:dir) | ||
return s:system(s:is_win | ||
\ ? 'rmdir /S /Q '.plug#shellescape(a:dir) | ||
\ : ['rm', '-rf', a:dir]) | ||
endif | ||
endfunction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Double-check the s:rm_rf(dir)
directory removal
The s:rm_rf
function forcibly removes directories. While this is expected behavior for cleaning up unneeded plugins, verify that the path calculation is safe and correct. Inadvertently passing an unintended directory can cause data loss. Consider additional checks or user confirmations (especially outside typical plugin paths).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example how it could be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivuorinen Here's an example of how to make the s:rm_rf
function safer:
function! s:rm_rf(dir)
if !isdirectory(a:dir)
return
endif
" Get absolute paths
let dir = fnamemodify(a:dir, ':p')
let plug_home = fnamemodify(g:plug_home, ':p')
" Ensure directory is within plug_home
if stridx(dir, plug_home) != 0
call s:err('Cannot remove directory outside plug_home: ' . dir)
return
endif
" Check for symlinks
if getftype(dir) ==# 'link'
call s:err('Cannot remove symlink: ' . dir)
return
endif
" Ask for confirmation
echohl WarningMsg
echo 'Remove directory and all contents?'
echo dir
echohl None
if s:ask('Proceed? (y/N) ') != 1
echo 'Cancelled.'
return
endif
return s:system(s:is_win
\ ? 'rmdir /S /Q ' . plug#shellescape(dir)
\ : ['rm', '-rf', dir])
endfunction
This improved version:
- Validates that the path exists and is a directory
- Ensures the directory is within
plug_home
to prevent accidental deletion of system files - Checks for and prevents removal of symlinks
- Prompts for user confirmation before deletion
- Uses absolute paths for safer comparison
The function will now fail safely if:
- The directory is outside the plugin directory
- The path is a symlink
- The user declines the confirmation
- The directory doesn't exist
(`・ω・´)ゞ
|
Summary by CodeRabbit
New Features
Chores
.gitignore
entry to exclude plugin-related files.Documentation