Support SDKMAN! Package Count#2033
Conversation
|
Does SDKMAN supports java SDKs only? We don't plan to add support for language specific package managers. |
|
From their website, I quote:
So, technically no. It is not only Java Development Kits (JDKs), but rather SDKs for Java Virtual Machine (JVM) and then some more. For example:
The Java equivalent for My point is: This is not to install Java project libraries like I laid my case. It is up to the maintainers and the community from here. |
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The implementation correctly identifies SDKMAN! candidates by looking for the 'current' directory and integrates the 'sdk' count into the output modules. However, the PR is currently missing logic to add these detected packages to the global package total (result->all), which will lead to inaccurate reporting.
While Codacy analysis is up to standards, the detection logic relies strictly on the SDKMAN_CANDIDATES_DIR environment variable. This should be improved with a fallback to the default installation path to support non-interactive shells. Additionally, the use of manual path construction should be replaced with the project's internal string buffer utilities for safety and consistency.
About this PR
- No unit or integration tests were provided to verify the SDKMAN! detection logic or handle edge cases such as missing environment variables or specific directory structures.
Test suggestions
- Detection returns 0 if the
SDKMAN_CANDIDATES_DIRenvironment variable is not set. - Detection correctly counts candidates that contain a 'current' directory and ignores those that do not.
- Hidden files or directories within the candidates path are ignored during the count.
- The detected 'sdk' count is included in the total packages count ('all') in the final output.
- Detection falls back to
$HOME/.sdkman/candidateswhen the environment variable is missing.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Detection returns 0 if the `SDKMAN_CANDIDATES_DIR` environment variable is not set.
2. Detection correctly counts candidates that contain a 'current' directory and ignores those that do not.
3. Hidden files or directories within the candidates path are ignored during the count.
4. The detected 'sdk' count is included in the total packages count ('all') in the final output.
5. Detection falls back to `$HOME/.sdkman/candidates` when the environment variable is missing.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| static uint32_t getSDKMAN(void) { | ||
| const char* candidatesDir = getenv("SDKMAN_CANDIDATES_DIR"); | ||
| if (!ffStrSet(candidatesDir)) | ||
| return 0; | ||
|
|
||
| FF_AUTO_CLOSE_DIR DIR* dir = opendir(candidatesDir); | ||
| if (!dir) | ||
| return 0; | ||
|
|
||
| uint32_t count = 0; | ||
| char path[PATH_MAX]; | ||
| struct dirent *entry; | ||
| while ((entry = readdir(dir)) != NULL) | ||
| { | ||
| if (entry->d_name[0] == '.') | ||
| continue; | ||
|
|
||
| if (entry->d_type == DT_DIR || entry->d_type == DT_UNKNOWN) | ||
| { | ||
| snprintf(path, sizeof(path), "%s/%s/current", candidatesDir, entry->d_name); | ||
| if (ffPathExists(path, FF_PATHTYPE_DIRECTORY)) | ||
| ++count; | ||
| } | ||
| } | ||
|
|
||
| return count; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Refactor the getSDKMAN function to improve robustness and consistency:
- Add a fallback check for the default path
~/.sdkman/candidates(usinginstance.state.platform.homeDir) ifSDKMAN_CANDIDATES_DIRis not set. - Use
ffStrbuforFF_STRBUF_AUTO_DESTROYinstead of manualchar[PATH_MAX]andsnprintfto avoid potential truncation and match the project's idiomatic style.
| { "Number of rpm packages", "rpm" }, | ||
| { "Number of scoop-global packages", "scoop-global" }, | ||
| { "Number of scoop-user packages", "scoop-user" }, | ||
| {"Number of SDKMAN! packages", "sdk"}, |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Add spaces inside the braces for consistency with the rest of the array: { "Number of SDKMAN! packages", "sdk" }.
2 participants
This Pull Request fulfills #2027.
Disclaimer: I have used generative AI to help me implement this feature. I have, however, had to thoroughly review, optimize, and fix the code myself.
Below is a screenshot of the result. Note that some data has been redacted from the screenshot.
Let me know if anything is missing.