Drop support for Vim prior to 8.2.5136 to fix CI on macOS #507
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces updates to several workflow configuration files for Neovim, Vim, and Reviewdog. Key changes include the addition of manual triggering capabilities via Changes
Possibly related issues
Possibly related PRs
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
.github/workflows/reviewdog.yml (1)
Line range hint
1-21: Consider the following improvements to enhance the workflow
The workflow name "reviewdog" might be slightly misleading as it specifically runs vint. Consider renaming it to "Vim Linting" or "Vint Check" for clarity.
To optimize when this workflow runs, you could add path filters. For example:
on: push: branches: - main paths: - '**/*.vim' - '**/*.vimrc' pull_request: paths: - '**/*.vim' - '**/*.vimrc' workflow_dispatch:This ensures the workflow only runs when Vim script files are changed.
Depending on your project needs, consider adding more linters or checks. For instance, if your project includes other languages alongside Vim script, you might want to add respective linters for those languages.
.github/workflows/vim.yml (1)
16-16: Matrix configuration updates are beneficial.The changes to the matrix configuration are good improvements:
- Using
macos-11instead ofmacos-latestprovides more control over the specific macOS version used in the workflow.- Changing
headtonightlyis more descriptive and aligns better with common terminology for the latest development version.Consider adding a comment explaining why
macos-11is used instead ofmacos-latest. This can help future maintainers understand the reasoning behind this specific version choice.Also applies to: 20-20
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/neovim.yml (2 hunks)
- .github/workflows/reviewdog.yml (1 hunks)
- .github/workflows/vim.yml (1 hunks)
Additional comments not posted (10)
.github/workflows/reviewdog.yml (2)
8-8: LGTM: Addition of manual workflow triggerThe addition of the
workflow_dispatchevent is a good practice. It allows for manual triggering of the workflow, which can be useful for on-demand runs or troubleshooting.
15-15: LGTM: Update of actions/checkout actionUpdating the
actions/checkoutaction from v2 to v4 is a good practice to leverage the latest features and bug fixes.To ensure compatibility with other actions in the workflow, please run the following script:
Verification successful
Verification Successful:
actions/checkoutis consistently updated to v4 across all workflows.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any potential incompatibilities with the updated actions/checkout@v4 # Test: Look for any other actions that might have specific version requirements rg --type yaml 'uses:\s+actions/checkout@' .github/workflowsLength of output: 374
.github/workflows/neovim.yml (4)
1-1: LGTM: Improved workflow name.The capitalization of "Neovim" in the workflow name enhances readability and follows proper naming conventions.
8-8: Excellent addition of manual trigger.The
workflow_dispatchevent allows manual triggering of this workflow, which is beneficial for debugging purposes or running the workflow on-demand without needing to push changes.
29-29: Verify the new Vim setup action.The change from
thinca/action-setup-vim@v1torhysd/action-setup-vim@v1appears to be a switch to a different maintainer or fork of the setup action.Please confirm that
rhysd/action-setup-vim@v1is reliable, actively maintained, and provides the necessary functionality for this workflow. You may want to check:
- The GitHub repository of the new action for recent updates and documentation.
- Any differences in features or usage between the old and new actions.
#!/bin/bash # Description: Fetch information about the new action # Test: Get basic info about the rhysd/action-setup-vim repository gh repo view rhysd/action-setup-vim --json name,description,createdAt,updatedAt,isArchived,stargazerCount # Test: Check for recent commits to ensure it's actively maintained gh api repos/rhysd/action-setup-vim/commits --jq '.[0:5] | .[] | {sha: .sha, date: .commit.author.date, message: .commit.message}'
32-33: LGTM: Updated action parameters.The changes to the action parameters appear to align with the new
rhysd/action-setup-vim@v1action's API:
- Replacing
vim_typewithneovim: trueexplicitly specifies Neovim setup.- Changing
vim_versiontoversionuses a more generic parameter name.These updates seem appropriate for the workflow's purpose.
To ensure these parameter changes are correct, please verify the documentation of the
rhysd/action-setup-vim@v1action. You can check the README of the action's repository:#!/bin/bash # Description: Fetch the README of the action repository # Test: Get the README content of the rhysd/action-setup-vim repository gh api repos/rhysd/action-setup-vim/readme --jq '.content' | base64 -d.github/workflows/vim.yml (4)
1-1: Workflow name and trigger updates look good.The workflow name change to "Vim" is more concise and appropriate. Adding the
workflow_dispatchevent is a good improvement as it allows manual triggering of the workflow, which can be useful for debugging or running the workflow on-demand.Also applies to: 8-8
11-11: Job name change is appropriate.Changing the job name from "build" to "test" more accurately reflects the purpose of this job, which is running tests rather than building. This improves the clarity of the workflow.
Line range hint
1-43: Confirm the removal of Linux build requirement comments.The comments regarding Linux build requirements have been removed from the workflow file. While this can improve readability by removing potentially outdated information, please confirm that these comments are no longer relevant to the build process.
To ensure that the removal of these comments doesn't impact the build process, please run the following verification script:
#!/bin/bash # Description: Check for any remaining references to Linux-specific build requirements echo "Checking for Linux-specific build requirements in other workflow files..." rg --type yaml "Linux.*build.*requirements" .github/workflows echo "Checking for Linux-specific build steps in this workflow..." rg --type yaml "apt-get|sudo|linux" .github/workflows/vim.yml echo "Please review the output to confirm that there are no remaining references to Linux-specific build requirements that might be impacted by the removal of these comments."
30-30: Vim setup action change looks good, but verify functionality.The change from
thinca/action-setup-vim@v1torhysd/action-setup-vim@v1and the simplification of input parameters to justversionappear to be improvements. This change likely provides better maintenance or features and simplifies the configuration.To ensure that the new action works as expected with the simplified input, please run the following verification script:
Also applies to: 33-33
Verification successful
The previous verification script encountered a syntax error. Please run the corrected script below to verify the new Vim setup action's functionality.
Please run the updated verification script below to confirm that the new Vim setup action is available and correctly accepts the
versioninput.
Vim setup action verified and functioning correctly.
The change from
thinca/action-setup-vim@v1torhysd/action-setup-vim@v1and the simplification of input parameters to justversionhave been confirmed to work as expected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the functionality of the new Vim setup action # Test: Check if the action is publicly available and its README echo "Checking the new Vim setup action..." gh repo view rhysd/action-setup-vim # Test: Verify if the action accepts the 'version' input echo "Verifying action.yml for 'version' input..." gh api repos/rhysd/action-setup-vim/contents/action.yml | jq -r '.content' | base64 -d | grep -A 5 "inputs:" echo "Please review the output to confirm that the action is available and accepts the 'version' input as expected."Length of output: 855
Script:
#!/bin/bash # Description: Verify the functionality of the new Vim setup action # Test: Check if the action is publicly available and its README echo "Checking the new Vim setup action..." gh repo view rhysd/action-setup-vim # Test: Verify if the action accepts the 'version' input echo "Verifying action.yml for 'version' input..." gh api repos/rhysd/action-setup-vim/contents/action.yml | jq -r '.content' | base64 -d | grep -A 5 "inputs:" echo "Please review the output to confirm that the action is available and accepts the 'version' input as expected."Length of output: 11009
Script:
#!/bin/bash # Description: Verify the functionality of the new Vim setup action # Test: Check if the action is publicly available and its README echo "Checking the new Vim setup action..." gh repo view rhysd/action-setup-vim # Test: Verify if the action accepts the 'version' input echo "Verifying action.yml for 'version' input..." gh api repos/rhysd/action-setup-vim/contents/action.yml -H "Accept: application/vnd.github.v3.raw" | grep -A 5 "inputs:" echo "Please review the output to confirm that the action is available and accepts the 'version' input as expected."Length of output: 11206
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/reviewdog.yml (1)
Line range hint
1-1: Consider using a more descriptive workflow nameWhile "reviewdog" accurately describes the tool being used, consider using a more descriptive name that includes the specific linter being run, such as "Vim Linting (vint)". This can make it easier to distinguish between different linting workflows at a glance.
Here's a suggested change:
-name: reviewdog +name: Vim Linting (vint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/neovim.yml (2 hunks)
- .github/workflows/reviewdog.yml (1 hunks)
- .github/workflows/vim.yml (2 hunks)
🧰 Additional context used
🔇 Additional comments (12)
.github/workflows/reviewdog.yml (1)
8-8: LGTM: Addition of manual triggerThe addition of the
workflow_dispatchtrigger is a good improvement. It allows for manual execution of the workflow, which can be beneficial for debugging or running the workflow on-demand without waiting for a push or pull request event..github/workflows/neovim.yml (6)
1-1: Improved workflow nameThe capitalization of "Neovim" in the workflow name is a good change. It improves readability and correctly represents the Neovim project name.
8-8: Manual trigger addedThe addition of the
workflow_dispatchtrigger is an excellent improvement. This allows manual execution of the workflow, which is particularly useful for testing and debugging purposes without the need for a push or pull request.
24-25: Checkout action updated, but duplication persistsThe update to
actions/checkout@v4is a positive change for potential security and feature improvements. However, as noted in a previous comment, there appears to be a duplicate use of this action. Please refer to the existing comment about removing one instance of the checkout action.
29-29: Vim setup action changedThe switch from
thinca/action-setup-vim@v1torhysd/action-setup-vim@v1has been noted. While this change may bring improvements or fixes, it's important to ensure that the new action is reliable and actively maintained.Please verify the reliability and maintenance status of the
rhysd/action-setup-vim@v1action. You can check its GitHub repository for recent updates, open issues, and community adoption.
32-33: Improved setup action parametersThe changes to the setup action parameters are beneficial:
- Replacing
vim_type: "Neovim"withneovim: truemakes the intention more explicit.- Renaming
vim_versiontoversionsimplifies the configuration.These modifications align well with the new
rhysd/action-setup-vim@v1action and improve the overall readability of the workflow.
Line range hint
1-33: Overall workflow improvementsThe changes to this Neovim workflow configuration file are generally positive:
- The workflow name has been improved.
- Manual triggering capability has been added.
- The checkout action has been updated to the latest version.
- The Vim setup action has been changed and its parameters have been optimized.
These updates enhance the workflow's flexibility and maintainability. The only remaining concern is the duplicate use of the checkout action, which has been addressed in a previous comment. Once that issue is resolved, this pull request should be ready for merging.
.github/workflows/vim.yml (5)
1-1: LGTM: Improved workflow nameThe capitalization of "Vim" in the workflow name is a good change. It improves readability and correctly represents the Vim editor name.
8-8: Excellent addition of manual triggerThe inclusion of the
workflow_dispatchtrigger is a valuable improvement. This allows for manual execution of the workflow, which can be particularly useful for on-demand testing or troubleshooting without needing to push changes or create a pull request.
11-11: Improved job naming and version testing
- Changing the job name from "build" to "test" more accurately describes the job's purpose, enhancing clarity.
- Updating the matrix strategy to use "nightly" instead of "head" ensures testing against the latest Vim changes, which is excellent for early detection of potential issues.
Both changes contribute to a more precise and effective workflow.
Also applies to: 20-20
25-26:⚠️ Potential issueRemove duplicate checkout action
While updating to
actions/checkout@v4is good, there's an unnecessary duplication of the checkout action. Unless there's a specific reason for the second checkout, it should be removed.Please apply the following change:
- uses: actions/checkout@v4 - - uses: actions/checkout@v4If the second checkout is intentional, please add a comment explaining its purpose.
30-33: Vim setup action update looks good, but needs clarificationThe changes to the Vim setup action appear to be improvements:
- Switching to
rhysd/action-setup-vim@v1- Removing unnecessary parameters
- Updating parameter naming to match the new action
These changes likely simplify the setup process and may provide better functionality.
Could you please clarify the reason for changing from
thinca/action-setup-vimtorhysd/action-setup-vim? This information would be helpful for understanding the benefits of the new action.To verify the new action's usage, you can run:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
plugin/fern.vim (1)
26-27: LGTM! Consider adding a comment explaining the version change.The update to the minimum supported Vim version (8.2.1119) is correctly implemented. The warning message has been appropriately updated to reflect this change.
Consider adding a brief comment explaining why this specific version (8.2.1119) was chosen as the new minimum supported version. This would provide valuable context for future maintainers.
+ " Vim 8.2.1119 is required because [brief explanation of the feature or fix introduced in this version] elseif !has('nvim') && !has('patch-8.2.1119') call s:warn('Vim prior to 8.2.1119 is not supported and fern might not work properly.')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/vim.yml (2 hunks)
- README.md (1 hunks)
- plugin/fern.vim (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/vim.yml
🧰 Additional context used
🔇 Additional comments (1)
plugin/fern.vim (1)
Line range hint
1-69: Verify documentation update for minimum Vim versionThe code has been updated to reflect the new minimum supported Vim version (8.2.1119). However, I couldn't find any changes in this file updating the documentation about this new requirement.
Please ensure that the README or any other relevant documentation has been updated to reflect this change in minimum supported Vim version. Run the following script to check for any mentions of Vim versions in the README:
If the script doesn't find any mentions or finds outdated information, please update the documentation accordingly.
lambdalisue
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/vim.yml (2)
20-21: LGTM: Improved version matrix with better terminology and specific versionThe changes to the matrix strategy are good:
- Using "nightly" instead of "head" is more standard terminology.
- Adding a specific version (v8.2.1313) helps address a known issue (as per the comment).
These improvements enhance the testing coverage and clarity of the workflow.
Consider adding a brief explanation in the comment about why this specific version is important, e.g.:
- v8.2.1313 # Minimum supported version due to CI issues on macOS (see #506)
24-27: LGTM: Added necessary dependency for macOS buildThe addition of the ncurses installation step for macOS when building Vim 8.2.1313 is a good solution to address the dependency issue. The conditional execution and explanatory comment enhance the workflow's robustness and maintainability.
Consider updating the comment to match the exact version being used:
# ncurses is required for building Vim 8.2.1313 on macOS
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/vim.yml (2 hunks)
- README.md (1 hunks)
- plugin/fern.vim (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- plugin/fern.vim
🧰 Additional context used
🔇 Additional comments (4)
.github/workflows/vim.yml (4)
1-1: LGTM: Improved workflow nameThe capitalization of "Vim" in the workflow name enhances readability and consistency with the product name.
8-8: LGTM: Added manual workflow triggerThe addition of the
workflow_dispatchtrigger allows for manual execution of the workflow, which is beneficial for debugging and on-demand testing.
11-11: LGTM: Job name accurately reflects its purposeChanging the job name from "build" to "test" more precisely describes the job's function, enhancing the workflow's clarity and maintainability.
33-36: LGTM: Updated Vim setup action with simplified configurationThe changes to the Vim setup action look good:
- Switching to
rhysd/action-setup-vim@v1action.- Removing the
vim_typeparameter.- Renaming
vim_versiontoversion.These updates simplify the configuration and likely provide improved functionality.
Please verify that the new action works as expected for all matrix combinations. Run the following script to check the action's usage across the repository:
lambdalisue
changed the title
lambdalisue
changed the title
See #506 for details.
SSIA
Summary by CodeRabbit
New Features
workflow_dispatch.Improvements
actions/checkoutfor enhanced performance.8.2.5136.Bug Fixes