Skip to content

devenv: support pkg as a type for --option#1988

Merged
domenkozar merged 1 commit into
mainfrom
pkg-cli-option
Jul 7, 2025
Merged

devenv: support pkg as a type for --option#1988
domenkozar merged 1 commit into
mainfrom
pkg-cli-option

Conversation

@sandydoo

@sandydoo sandydoo commented Jul 7, 2025

Copy link
Copy Markdown
Member

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
@claude

claude Bot commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

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.


@cloudflare-workers-and-pages

Copy link
Copy Markdown

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