BugFix: hook not executed when only config is updated #3
Conversation
|
Warning Rate limit exceeded@kesslerdev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DotLua
participant Module
User->>DotLua: Call process_module(module_name, options)
DotLua->>DotLua: Handle configuration symlink
DotLua->>DotLua: Check if config changed
alt Config Changed
DotLua->>Module: Run post-install/purge hooks
else No Change
DotLua->>User: No hooks to run
end
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: 0
🧹 Outside diff range and nitpick comments (1)
dot.lua (1)
373-373: LGTM! The boolean return value implementation looks good.The function now properly tracks configuration changes through the
config_changedflag and consistently returns boolean values. This change is essential for the PR objective of fixing hook execution when only config is updated.Consider adding a brief comment explaining the return value semantics:
local function handle_config_symlink(config, module_dir, options) + -- Returns true if any configuration changes occurred, false otherwise if not config.config then return false endAlso applies to: 378-378, 391-391, 409-409, 416-416, 439-439, 443-443, 451-451, 460-460, 474-474
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
dot.lua(7 hunks)
🔇 Additional comments (1)
dot.lua (1)
499-499: LGTM! The hook execution logic is now complete.
The updated condition properly handles all cases that should trigger hooks:
- When dependencies are changed
- When configuration is changed
- When hooks mode is explicitly enabled
This change successfully addresses the PR objective by ensuring hooks are executed when only configuration files are updated.
Also applies to: 502-502
dot.lua
Outdated
|
|
||
| -- Run post_install or post_purge hooks | ||
| if dependencies_changed or options.hooks_mode then | ||
| if dependencies_changed or config_changed or options.hooks_mode then |
There was a problem hiding this comment.
heyyy @kesslerdev thanks so much for the PR. I didn't make it like this cause a "post-install" hook in my head only runs when you "install" the dependencies. I guess there would be an argument for your use-case, but might be better to create a "post-link" hook. WDYT?
There was a problem hiding this comment.
Hey @pablopunk! Thanks for the feedback. I agree, a new hook like "post-link" (and also a "post-unlink") definitely fits the use case better. I’ve updated the docs and the tests to include these hooks, so everything’s good to go. Thanks again for the suggestion! 😊
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
dot.lua (1)
510-517: Consider aligning hook names with their purposeWhile the implementation correctly addresses the issue of hooks not running for config-only changes, consider the naming discussion from the previous review. The current implementation uses
post_configandpost_config_purge, but as suggested,post_linkandpost_unlinkmight better reflect their purpose, as these hooks specifically handle symlink operations.This would make the hook naming more intuitive:
post_install: for package installationpost_link: for symlink creation (currentlypost_config)post_unlink: for symlink removal (currentlypost_config_purge)post_purge: for package removal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
dot.lua(8 hunks)
🔇 Additional comments (3)
dot.lua (3)
373-373: LGTM: Consistent boolean return type
Good change to explicitly return false when no config is present, ensuring consistent return type and proper tracking of configuration changes.
Line range hint 378-391: LGTM: Proper tracking of configuration changes
Good implementation of configuration change tracking. The config_changed flag is properly set when configurations are removed, which is essential for determining when to run hooks.
409-409: LGTM: Improved error handling
Good improvement to error handling by explicitly returning false on failures. This ensures hooks don't run when operations fail, maintaining system consistency.
Also applies to: 439-439, 443-443, 451-451
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
dot.lua (1)
499-499: LGTM: Enhanced hook execution logicThe addition of
post_linkandpost_unlinkhooks provides better granularity for configuration-specific actions, addressing the original issue while maintaining semantic clarity. This aligns well with the suggestion from the previous review.Consider adding documentation to explain the difference between:
post_install(runs when dependencies change)post_link(runs when configurations change)Would you like me to help generate documentation for the hook types?
Also applies to: 510-517
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
dot.lua(8 hunks)
🔇 Additional comments (3)
dot.lua (3)
373-373: LGTM: Improved return value consistency
The changes properly track configuration modifications through the config_changed boolean, which is essential for determining when to run hooks. This fixes the core issue where hooks weren't being executed when only configurations were updated.
Also applies to: 378-378, 391-391, 416-416, 460-460, 474-474
409-409: LGTM: Improved error handling
The addition of explicit false returns on error conditions ensures proper error propagation and prevents further processing when errors occur. This makes the function more robust and reliable.
Also applies to: 439-439, 443-443, 451-451
Line range hint 499-517: Consider handling combined hook scenarios
Currently, post_install and post_link hooks run independently based on their respective triggers (dependencies_changed and config_changed). Consider whether there are scenarios where both types of changes occur, and if the hook execution order matters in such cases.
Let's check if any modules in the codebase use both types of hooks:
If modules using both hooks are found, consider:
- Documenting the hook execution order
- Adding a mechanism to share state between hooks if needed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
README.md (2)
231-239: Enhance hook documentation with trigger conditions.While the hooks are well documented, it would be helpful to specify the exact conditions that trigger each hook. For example, clarify that hooks run even when only configuration files are updated (which relates to the bug being fixed in this PR).
Consider adding trigger conditions like this:
- **`post_install`**: Runs after the specified packages have been installed via `brew`. + - Triggered after brew package installation or when configuration files are updated - **`post_purge`**: Runs after the specified packages have been removed via `brew`. + - Triggered during module purge, regardless of whether brew packages exist - **`post_link`**: Runs after configuration files have been linked. + - Triggered whenever configuration files are linked, even if no brew packages exist - **`post_unlink`**: Runs after configuration files have been unlinked. + - Triggered during unlink operations, independent of brew package status🧰 Tools
🪛 LanguageTool
[uncategorized] ~235-~235: Loose punctuation mark.
Context: ...## Available Hooks -post_install: Runs after the specified packages have ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~236-~236: Loose punctuation mark.
Context: ...installed viabrew. -post_purge: Runs after the specified packages have ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~237-~237: Loose punctuation mark.
Context: ...en removed viabrew. -post_link: Runs after configuration files have bee...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...es have been linked. -post_unlink: Runs after configuration files have bee...(UNLIKELY_OPENING_PUNCTUATION)
240-247: Enhance basic hook example with error handling.The current example is clear but could be improved to demonstrate error handling best practices.
Consider updating the example to show error handling:
return { brew = { "gh" }, - post_install = "gh auth login" + post_install = [[ + if ! gh auth status &>/dev/null; then + echo "Authenticating with GitHub..." + gh auth login || echo "Failed to authenticate with GitHub" + fi + ]] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~235-~235: Loose punctuation mark.
Context: ...## Available Hooks - post_install: Runs after the specified packages have ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~236-~236: Loose punctuation mark.
Context: ...installed via brew. - post_purge: Runs after the specified packages have ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~237-~237: Loose punctuation mark.
Context: ...en removed via brew. - post_link: Runs after configuration files have bee...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...es have been linked. - post_unlink: Runs after configuration files have bee...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (1)
README.md (1)
Line range hint 248-261: LGTM! Well-documented multi-line hook example.
The multi-line hook example effectively demonstrates:
- Proper error handling with status checks
- Use of Lua's multi-line string syntax
- Real-world usage with GitHub CLI extensions
🧰 Tools
🪛 LanguageTool
[uncategorized] ~235-~235: Loose punctuation mark.
Context: ...## Available Hooks - post_install: Runs after the specified packages have ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~236-~236: Loose punctuation mark.
Context: ...installed via brew. - post_purge: Runs after the specified packages have ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~237-~237: Loose punctuation mark.
Context: ...en removed via brew. - post_link: Runs after configuration files have bee...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...es have been linked. - post_unlink: Runs after configuration files have bee...
(UNLIKELY_OPENING_PUNCTUATION)
This fix adjusts the logic to ensure that hooks are correctly called, even for modules containing only configuration files.
Now, all modules, whether they contain brew package or only configurations, will have their hooks called.
Summary by CodeRabbit
New Features
Bug Fixes