Skip to content

Conversation

sandydoo
Copy link
Member

@sandydoo sandydoo commented Jul 7, 2025

For example:

devenv shell --option "languages.java.jdk.package" "graalvm-oracle"

Fixes #1987.

For example:

`devenv shell --option "languages.java.jdk.package" "graalvm-oracle"`
@sandydoo sandydoo added the enhancement New feature or request label Jul 7, 2025
Copy link
Contributor

claude bot commented Jul 7, 2025

Claude finished @sandydoo's task —— View job


PR Review: Support pkg as a type for --option

Todo List:

  • Read and analyze the changed files
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Review test coverage
  • Provide constructive feedback

Analysis:

The PR adds support for pkg as a type for the --option CLI flag. Here's what I found:

Files Changed:

  1. devenv/src/cli.rs:169 - Added "pkg" to the help text for supported types
  2. devenv/src/devenv.rs:1078-1079 - Added "pkg" to SUPPORTED_TYPES array
  3. devenv/src/devenv.rs:1099 - Added implementation: "pkg" => format!("pkgs.{}", &chunk[1])

How it works:

  • When using --option "languages.java.jdk.package:pkg" "graalvm-oracle", it formats the value as pkgs.graalvm-oracle
  • This allows referencing single packages, similar to how pkgs type works for multiple packages

Code Quality: ✅ Good

  • Implementation is consistent with existing patterns
  • Follows established naming conventions
  • Clear and maintainable code

Potential Issues: ⚠️ Minor concerns

  • No input validation for package names (though this is consistent with other types)
  • Could potentially allow malformed package references

Performance: ✅ No impact

  • Simple string formatting operation
  • No loops or heavy operations added

Security: ✅ No concerns

  • Just string formatting, no shell execution or file operations
  • No injection risks identified

Test Coverage: ❌ Missing

  • No tests exist for the new pkg type functionality
  • The existing tests/cli-options/ directory doesn't contain any test scripts
  • Should add tests to verify the pkg type works correctly

Constructive Feedback

Missing Test Coverage:
The biggest concern is the lack of test coverage for the new pkg type. Consider adding:

  1. Unit tests for the option parsing logic (similar to the verify_cli() test in cli.rs:448)
  2. Integration tests in the tests/cli-options/ directory to verify:
    • --option "some.path:pkg" "package-name" correctly generates pkgs.package-name
    • Error handling for malformed package references
    • Comparison with existing pkgs type behavior

Suggested Test Structure:

# Add to tests/cli-options/
echo '#!/bin/bash
# Test pkg type option
devenv shell --option "languages.java.jdk.package:pkg" "graalvm-oracle" --command "echo \$JAVA_HOME"
' > tests/cli-options/.test.sh

Documentation:
Consider updating the documentation to include an example of the new pkg type usage, similar to the existing examples in the help text.

Input Validation:
While not critical, you might consider adding basic validation for package names (e.g., checking for valid characters) to provide better error messages for typos.


Copy link

Deploying devenv with  Cloudflare Pages  Cloudflare Pages

Latest commit: 10eddfd
Status: ✅  Deploy successful!
Preview URL: https://ece6d06d.devenv.pages.dev
Branch Preview URL: https://pkg-cli-option.devenv.pages.dev

View logs

@domenkozar domenkozar merged commit ae22852 into main Jul 7, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI --option should accept :pkg type
2 participants