SED-4497-referencing-a-keyword-from-an-ap-library-doesnt-work#120
SED-4497-referencing-a-keyword-from-an-ap-library-doesnt-work#120david-stephan wants to merge 2 commits intomasterfrom
Conversation
Summary of ChangesHello, 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! This pull request addresses an issue where keywords from external libraries were not being correctly referenced. It enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new utility method getClasspathElementUrl to the AnnotationScanner class. This method allows retrieving the source URL (e.g., JAR file) of a class from the classpath scan results. The implementation is correct and includes good documentation. However, the new public method is missing corresponding unit tests. Adding tests is crucial to validate its behavior and prevent future regressions. Additionally, since CachedAnnotationScanner acts as a cached wrapper for AnnotationScanner, you may want to consider exposing this new functionality through it as well for consistency.
| public URL getClasspathElementUrl(String className) { | ||
| ClassInfo classInfo = scanResult.getClassInfo(className); | ||
| return classInfo != null ? classInfo.getClasspathElementURL() : null; | ||
| } |
There was a problem hiding this comment.
This new public method getClasspathElementUrl is not covered by unit tests. Please add a new test to AnnotationScannerTest.java to validate its functionality. A good test case would verify that a URL is returned for a known class and that null is returned for a class that does not exist in the scan results.
There was a problem hiding this comment.
Code Review
This pull request introduces a new method getClasspathElementUrl to the AnnotationScanner class, which allows retrieving the classpath URL for a given class name. The implementation is straightforward, but I've suggested an improvement to handle null input gracefully. Additionally, the new public method should be accompanied by unit tests to ensure its correctness and prevent future regressions.
| * @param className the binary class name (e.g. {@code "com.example.MyKeywords"}) | ||
| * @return the URL of the JAR or directory that contains the class, or {@code null} | ||
| */ | ||
| public URL getClasspathElementUrl(String className) { |
There was a problem hiding this comment.
This new public method should be covered by unit tests in AnnotationScannerTest.java. Please add tests for at least the following cases:
- A class that is found in the scan result.
- A class that is not found in the scan result (should return
null). - A
nullclass name.
There was a problem hiding this comment.
Junit tests added
| ClassInfo classInfo = scanResult.getClassInfo(className); | ||
| return classInfo != null ? classInfo.getClasspathElementURL() : null; |
There was a problem hiding this comment.
The scanResult.getClassInfo(className) method from the classgraph library will throw an IllegalArgumentException if className is null. To make this method more robust, it's a good practice to handle the null case explicitly by returning null, which is consistent with the behavior when a class is not found.
if (className == null) {
return null;
}
ClassInfo classInfo = scanResult.getClassInfo(className);
return classInfo != null ? classInfo.getClasspathElementURL() : null;There was a problem hiding this comment.
Classname value check added, but we do not accept null value and now throw an IllegalArgumentException
1 participant
No description provided.