Skip to content

Conversation

domenkozar
Copy link
Member

No description provided.

Copy link
Contributor

claude bot commented Jul 25, 2025

Claude finished @domenkozar's task —— View job


Code Review: PR #2039 - Stack Nix Integration Changes

Todo List:

  • Read and analyze the changed file (src/modules/languages/haskell.nix)
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Look for security concerns
  • Evaluate test coverage
  • Provide constructive feedback

Issues Found

Code Quality & Style Issues

1. Formatting Issue (Line 47)

description = '' Whether to enable the Haskell Stack      '';

There are unnecessary trailing spaces. Should be:

description = "Whether to enable the Haskell Stack";

2. Potential Runtime Error (Line 16)

ghcVersion = lib.replaceStrings [ "." ] [ "" ] cfg.package.version;

This assumes cfg.package.version exists and is a string. If the GHC package doesn't have a .version attribute or it's null, this will fail. Consider adding error handling:

ghcVersion = lib.replaceStrings [ "." ] [ "" ] (cfg.package.version or "");

3. Stack Arguments Validation (Lines 61-62)
The stack.args option accepts any list of strings without validation. Malformed Stack arguments could cause confusing runtime errors. Consider adding validation or at least documenting expected format.

Missing Test Coverage

4. No Dedicated Tests
The Haskell module lacks specific tests. Only found basic enablement in examples/supported-languages/devenv.nix. Consider adding:

  • Test that verifies Stack wrapper is created correctly
  • Test that validates Stack args are properly passed
  • Test for different GHC versions
  • Integration test that actually runs Stack commands

Documentation Gaps

5. Limited Documentation
The options have basic descriptions but lack:

  • Examples of common stack.args configurations
  • Guidance on GHC version compatibility with HLS
  • Migration guide from existing Stack setups

Performance Considerations

6. Stack Wrapper Recreation
The stackWrapper derivation is recreated whenever cfg.stack.args changes, which is appropriate but worth noting that frequent changes will invalidate the build cache.

Security Assessment

No security issues identified. The implementation uses standard Nix practices and doesn't introduce privilege escalation or unsafe operations.

Recommendations

  1. Fix the formatting issue on line 47
  2. Add error handling for GHC version extraction
  3. Add comprehensive tests for the module
  4. Consider input validation for stack.args
  5. Improve documentation with examples

Overall, this is a solid implementation that addresses the core requirement of configuring Stack to use devenv's GHC. The main concerns are around robustness and test coverage.

Copy link

cloudflare-workers-and-pages bot commented Jul 25, 2025

Deploying devenv with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3dca65f
Status: ✅  Deploy successful!
Preview URL: https://b051a2f9.devenv.pages.dev
Branch Preview URL: https://stack-no-nix.devenv.pages.dev

View logs

Copy link
Contributor

🔍 Suggested Reviewers

Based on git blame analysis of the changed lines, the following contributors have significant experience with the modified code:

Please consider reviewing this PR as you have authored significant portions of the code being modified. Your expertise would be valuable! 🙏

This comment was automatically generated by git-blame-auto-reviewer

Last updated: 2025-07-28T21:40:50.966Z

@domenkozar domenkozar merged commit 3f1197a into main Jul 28, 2025
7 of 8 checks passed
sandydoo pushed a commit that referenced this pull request Aug 21, 2025
stack: instruct it to not use nix as we provide ghc from devenv
sandydoo pushed a commit that referenced this pull request Aug 21, 2025
stack: instruct it to not use nix as we provide ghc from devenv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant