refactor: move http_jar test dependencies behind a macro
#79
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: https://app.greptile.com/review/github.
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
WalkthroughThe changes refactor how test and development dependencies are managed for the Bazel rules_antlr project. Explicit Changes
Sequence Diagram(s)sequenceDiagram
participant User_WORKSPACE
participant repositories_bzl
User_WORKSPACE->>repositories_bzl: rules_antlr_dependencies(...)
User_WORKSPACE->>repositories_bzl: rules_antlr_test_dependencies()
repositories_bzl->>repositories_bzl: _maven_jars(test_deps, repository)
repositories_bzl->>repositories_bzl: maybe(http_jar) for each test dependency
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
albertocavalcante
temporarily deployed
to
secrets-enabled
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @albertocavalcante, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. This PR, authored by albertocavalcante, aims to refactor how test dependencies are managed within the rules_antlr Bazel project. Specifically, it moves the http_jar declarations for test-related libraries (junit, jimfs, guava) out of the main WORKSPACE.bazel file and into a dedicated macro within antlr/repositories.bzl. This change centralizes the definition and loading of these dependencies, making the WORKSPACE file cleaner and ensuring these test dependencies are loaded using the maybe() function to prevent potential conflicts if users of rules_antlr already load these libraries.
Highlights
- Dependency Management Refactor: Moves test dependencies (
junit,jimfs,guava) fromWORKSPACE.bazelinto a new macro inantlr/repositories.bzl. - New Macro for Test Dependencies: Introduces
rules_antlr_test_dependencies()inantlr/repositories.bzlto encapsulate the loading of test-specific external dependencies. - Use of
maybe(): Ensures test dependencies are loaded usingmaybe(http_jar, ...)to avoid conflicts if these dependencies are already defined elsewhere in a user'sWORKSPACE. - Centralized Maven URL Construction: Adds a helper function
_maven_jarsto construct standard Maven Central URLs based on group, artifact, and version, used by the new test dependencies macro.
Changelog
- WORKSPACE.bazel
- Removed direct
http_jardeclarations forjunit,jimfs, andguava(lines 54-70). - Added
rules_antlr_test_dependenciesto theloadstatement for//antlr:repositories.bzl(line 54). - Added a call to the new
rules_antlr_test_dependencies()macro (line 59).
- Removed direct
- antlr/repositories.bzl
- Added
loadstatement formaybefrom@bazel_tools//tools/build_defs/repo:utils.bzl(line 4). - Defined
_MAVEN_CENTRALconstant for the Maven Central URL (line 7). - Updated the internal
_downloadfunction to usemaybe(http_jar, ...)instead ofhttp_jar(...)and use the_MAVEN_CENTRALconstant (lines 611-616). - Added the public macro
rules_antlr_test_dependenciesto load test dependencies (lines 638-678). - Added the internal helper function
_maven_jarsto handle loading multiple Maven JARs usingmaybe()and constructing URLs (lines 680-700).
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the way test dependencies (junit, jimfs, guava) are declared by moving them from direct http_jar calls in WORKSPACE.bazel into a new macro rules_antlr_test_dependencies within antlr/repositories.bzl.
This is a good improvement for several reasons:
- Centralization: Test dependency definitions are now centralized in
antlr/repositories.bzl, making them easier to manage and update. - Clarity: It clearly separates user-facing dependencies (loaded by
rules_antlr_dependencies) from internal test/development dependencies. - Maintainability: The new
_maven_jarshelper function provides a structured way to define Maven artifacts using group, artifact, and version, and automatically constructs the download URL. - Robustness: The use of
maybe(http_jar, ...)prevents errors if repositories are declared multiple times. - Consistency: The
_downloadfunction was also updated to usemaybeand the_MAVEN_CENTRALconstant, improving consistency.
The changes are well-implemented, and the new macro rules_antlr_test_dependencies is clearly documented. Overall, this enhances the organization and maintainability of the project's external dependencies.
Summary of Findings
- URL Construction Readability in
_maven_jars: In the_maven_jarsfunction (antlr/repositories.bzl, lines 691-698), the Maven URL is constructed using a singleformat()call with multiple placeholders. While this is correct and functional, for very complex URLs or a larger number of placeholders, breaking down the path construction into intermediate variables or using named placeholders informat()(if Starlark'sformatsupported it more like Python's advanced features) could sometimes offer a slight improvement in readability. For the current complexity, the existing approach is perfectly acceptable. This is a minor stylistic observation of low severity and does not require changes in this PR. As per review settings, a direct comment was not added for this low-severity item.
Merge Readiness
The changes in this pull request are well-executed and represent a clear improvement in how test dependencies are managed. No critical, high, or medium severity issues were found. The code appears to be in good shape. While I cannot approve the pull request myself, based on this review, it seems ready for merging after any standard internal review processes are completed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
WORKSPACE.bazel(1 hunks)antlr/repositories.bzl(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (6)
WORKSPACE.bazel (2)
54-54: LGTM! Good separation of concerns.Loading the test dependencies macro alongside the main dependencies follows a clean pattern and improves modularity.
58-59: LGTM! Clear documentation and proper separation.The comment clearly explains that these are test/development dependencies not needed by users, and the call follows the established pattern.
antlr/repositories.bzl (4)
4-4: LGTM! Standard Bazel pattern for preventing conflicts.Adding the
maybeutility import is a best practice to avoid duplicate repository declarations.
7-7: LGTM! Good centralization of the Maven Central URL.Extracting the Maven Central URL into a constant improves maintainability and reusability.
611-616: LGTM! Improved conflict prevention and URL handling.The
maybewrapper prevents duplicate repository declarations, and the URL construction properly handles both absolute URLs and Maven paths.
680-700: LGTM! Well-designed helper function with proper Maven URL construction.The
_maven_jarshelper function follows best practices:
- Uses
maybe()to prevent conflicts- Correctly constructs Maven Central URLs
- Configurable repository parameter
- Clear parameter documentation
The URL format
{repo}/{group}/{artifact}/{version}/{artifact}-{version}.jarmatches Maven Central conventions.
1 participant
Summary by CodeRabbit