Replace nixpkgs-fmt with nixfmt #97
Conversation
WalkthroughThis pull request introduces significant updates across multiple files, primarily focusing on replacing the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
src/ext/scriv.nix (1)
5-6: Consider improving parameter formatting for better readability.While the parameter reordering is functionally correct (Nix named parameters are position-independent), the comma placement at the end of line 5 makes the code less readable.
Consider this format instead:
- package = { lib, buildPythonPackage, fetchPypi, attrs, click, click-log - , requests, jinja2 }: + package = { + lib + , buildPythonPackage + , fetchPypi + , click + , click-log + , requests + , jinja2 + , attrs + }:src/core/mkMergedShellProfiles.nix (2)
30-34: Consider implementing a unified clash detection strategy.The tools and preCommit configurations have the same silent override behavior as scripts and env. Consider implementing a consistent clash detection strategy across all attribute sets.
I suggest creating a generic merge function that handles clashes uniformly:
let # Generic merge function with clash detection mergeAttrsWithClash = name: attr1: attr2: let commonKeys = builtins.intersectAttrs attr1 attr2; hasClashes = (builtins.length (builtins.attrNames commonKeys)) > 0; in if hasClashes then throw "Attribute clash in ${name}: ${toString (builtins.attrNames commonKeys)}" else attr1 // attr2; in { tools = mergeAttrsWithClash "tools" (utils.getAttrWithDefault "tools" { } p1) (utils.getAttrWithDefault "tools" { } p2); preCommit = mergeAttrsWithClash "preCommit" (utils.getAttrWithDefault "preCommit" { } p1) (utils.getAttrWithDefault "preCommit" { } p2); }Would you like me to help implement this unified approach?
Also applies to: 36-40
Line range hint
1-45: Consider restructuring for better error handling and configuration.While the current implementation works, consider these architectural improvements:
- Add a configuration option to control merge behavior (error on clash vs. override)
- Implement proper error reporting with meaningful messages
- Add documentation about merge behavior and potential conflicts
Example configuration structure:
{ mergeConfig = { errorOnClash = true; pathSeparator = ":"; overrideStrategy = "error"; # or "override" or "prefix" }; }src/ext/haskell-language-server-project.nix (1)
82-86: Consider adding inline documentationThe fallback logic ensures consistent versions of hlint and stylish-haskell across different GHC versions. Consider adding a brief comment explaining why this override is necessary, similar to the detailed comment above in the
projectdefinition.- project' = let hls-fallback = config.hls-fallback or project; + # Ensure consistent versions of formatting tools across different GHC versions + # by using the fallback project's versions when specified + project' = let hls-fallback = config.hls-fallback or project;src/core/mkShellUtilityScripts.nix (3)
25-37: Consider simplifying the formatGroup functionWhile functionally correct, the nested conditionals could be simplified for better readability.
Consider this alternative structure:
- formatGroup = group: command: - if lib.hasAttr group user-inputs.self && user-inputs.self.${group} - != { } then - let - mkCommand = name: _: "nix ${command} .#${utils.ansiBold name}"; - commands = - lib.mapAttrsToList mkCommand user-inputs.self.${group}.${system}; - in '' - ${utils.ansiColor group "purple" "bold"} - - ${lib.concatStringsSep "\n" commands}'' - else - ""; + formatGroup = group: command: + let + hasValidOutputs = lib.hasAttr group user-inputs.self && + user-inputs.self.${group} != { }; + mkCommand = name: _: "nix ${command} .#${utils.ansiBold name}"; + in + if !hasValidOutputs then "" else + let + commands = lib.mapAttrsToList mkCommand user-inputs.self.${group}.${system}; + in '' + ${utils.ansiColor group "purple" "bold"} + + ${lib.concatStringsSep "\n" commands}'';
81-82: Consider making internal variables configurableThe internal variables are currently hardcoded. Consider making them configurable through the module system for better flexibility.
- internal-vars = - [ "NIX_GHC_LIBDIR" "PKG_CONFIG_PATH" "CABAL_CONFIG" "LOCALE_ARCHIVE" ]; + internal-vars = mkShell-IN.internalVars or + [ "NIX_GHC_LIBDIR" "PKG_CONFIG_PATH" "CABAL_CONFIG" "LOCALE_ARCHIVE" ];
97-98: Improve string concatenation readabilityThe string concatenation could be more readable using Nix's string interpolation.
- content = lib.optionalString (formatted-env != "") formatted-env - + "${formatted-script-groups}"; + content = '' + ${lib.optionalString (formatted-env != "") formatted-env} + ${formatted-script-groups}'';src/lib/utils.nix (1)
90-103: Consider enhancing the source attribution commentWhile the formatting changes improve readability, consider adding more details to the source attribution:
- Add the specific commit or version from which this was taken
- Include the license information if available
src/core/mkHaskellProject.nix (2)
52-59: LGTM: Clear and actionable error messageThe error message effectively communicates both the problem and the solution. Consider adding bullet points for better readability when multiple duplicates are listed.
- ${lib.concatStringsSep " " duplicates} + ${lib.concatMapStrings (d: "• ${d}\n") duplicates}
91-107: LGTM: Robust shell configuration mergingThe implementation correctly combines shell configurations while preserving important attributes. Consider adding a brief comment explaining why this implementation exists alongside
mkProjectDevShell.Add a comment above the function:
# Alternative implementation that preserves pre-commit-check and tools # while properly merging shell configurations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (39)
CHANGELOG.md(1 hunks)doc/api.md(4 hunks)flake.nix(5 hunks)src/core/mkCombinedHaddock.nix(2 hunks)src/core/mkContainerFromCabalExe.nix(2 hunks)src/core/mkGitRevProjectOverlay.nix(1 hunks)src/core/mkHaskellProject.nix(6 hunks)src/core/mkHydraRequiredJob.nix(1 hunks)src/core/mkMergedShellProfiles.nix(1 hunks)src/core/mkReadTheDocsShellProfile.nix(2 hunks)src/core/mkReadTheDocsSite.nix(2 hunks)src/core/mkShellUtilityScripts.nix(1 hunks)src/core/mkShellWith.nix(5 hunks)src/ext/cabal-fmt.nix(1 hunks)src/ext/fourmolu.nix(1 hunks)src/ext/haskell-language-server-project.nix(3 hunks)src/ext/nixpkgs-fmt.nix(0 hunks)src/ext/scriv.nix(3 hunks)src/ext/sphinx-markdown-tables.nix(1 hunks)src/ext/sphinx-toolchain.nix(1 hunks)src/ext/sphinxemoji.nix(1 hunks)src/lib/modularise.nix(3 hunks)src/lib/utils.nix(3 hunks)src/mkFlake.nix(3 hunks)src/options/default.nix(1 hunks)src/options/flake-dot-nix.nix(1 hunks)src/options/mkContainerFromCabalExe.nix(3 hunks)src/options/mkFlake.nix(10 hunks)src/options/mkGitRevOverlay.nix(1 hunks)src/options/mkHaskellProject.nix(12 hunks)src/options/mkHydraRequiredJob.nix(1 hunks)src/options/mkShell.nix(18 hunks)templates/haskell/flake.nix(1 hunks)templates/haskell/nix/outputs.nix(1 hunks)templates/haskell/nix/project.nix(3 hunks)templates/haskell/nix/shell.nix(2 hunks)templates/vanilla/flake.nix(1 hunks)templates/vanilla/nix/outputs.nix(1 hunks)templates/vanilla/nix/shell.nix(2 hunks)
💤 Files with no reviewable changes (1)
- src/ext/nixpkgs-fmt.nix
✅ Files skipped from review due to trivial changes (23)
- flake.nix
- src/core/mkCombinedHaddock.nix
- src/core/mkContainerFromCabalExe.nix
- src/core/mkHydraRequiredJob.nix
- src/core/mkReadTheDocsShellProfile.nix
- src/ext/cabal-fmt.nix
- src/ext/fourmolu.nix
- src/ext/sphinx-markdown-tables.nix
- src/ext/sphinx-toolchain.nix
- src/ext/sphinxemoji.nix
- src/lib/modularise.nix
- src/mkFlake.nix
- src/options/default.nix
- src/options/mkContainerFromCabalExe.nix
- src/options/mkGitRevOverlay.nix
- src/options/mkHydraRequiredJob.nix
- templates/haskell/flake.nix
- templates/haskell/nix/outputs.nix
- templates/haskell/nix/project.nix
- templates/haskell/nix/shell.nix
- templates/vanilla/flake.nix
- templates/vanilla/nix/outputs.nix
- templates/vanilla/nix/shell.nix
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
8-8: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
16-16: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (49)
src/core/mkGitRevProjectOverlay.nix (2)
17-18: LGTM: Formatting changes preserve functionality
The reformatting of the hsPkgs assignment maintains the original logic while improving readability. The setGitRevForPaths function is still correctly applied with the same parameters.
22-22: LGTM: Cosmetic improvement
The simplified formatting of the overlay append expression enhances readability while maintaining the same functionality.
src/ext/scriv.nix (1)
19-19: LGTM!
The single-line format for propagatedBuildInputs is clean and readable given the small number of dependencies.
src/core/mkMergedShellProfiles.nix (2)
10-11: LGTM!
The package merging logic correctly concatenates the package lists from both profiles, with proper default value handling.
25-28: LGTM!
The shellHook merging logic correctly concatenates hooks with proper newline separation.
src/core/mkReadTheDocsSite.nix (2)
18-31: LGTM! Improved readability of file suffixes list.
The reformatting of source file suffixes from single-line to multi-line format enhances code readability while maintaining the same functionality.
56-56: LGTM! Style aligned with nixfmt-classic.
The repositioning of the in keyword aligns with the new formatter's style guidelines while preserving the conditional logic.
src/ext/haskell-language-server-project.nix (2)
70-71: LGTM!
The SHA256 map entry is correctly formatted and includes the necessary editor config override for the long line.
31-32: Verify the status of GHC 9.10 support
There's a TODO comment indicating incomplete HLS support for GHC 9.10 with hlint and stylish-haskell. Consider:
- Tracking this limitation in the project's issues
- Adding a timeline for when full support is expected
✅ Verification successful
Let me try a different search strategy to verify the GHC 9.10 support status.
GHC 9.10 support is properly handled with a documented fallback
The codebase has a well-structured fallback mechanism for GHC 9.10:
- The fallback to GHC 9.8 is explicitly configured in
haskell-language-server-project.nix - The setup is properly integrated in
flake.nixwith a dedicatedghc910-shell - The TODO comment accurately documents the temporary nature of this workaround
No action is needed as this is a valid temporary solution until upstream HLS fully supports GHC 9.10 with hlint and stylish-haskell.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's an existing issue tracking GHC 9.10 support
gh issue list --search "ghc910 hlint stylish-haskell support"
# Check the latest discussions about GHC 9.10 support in HLS repository
gh api graphql -f query='
{
search(query: "repo:haskell/haskell-language-server ghc910 hlint stylish-haskell", type: ISSUE, first: 5) {
nodes {
... on Issue {
title
url
state
}
}
}
}'
Length of output: 332
Script:
#!/bin/bash
# Search for any mentions of GHC 9.10 or ghc910 in the repository
rg -i "ghc.?9\.10|ghc910" -A 2 -B 2
# Check if there are any issues or tracking items in project files
fd "TODO|FIXME|NOTE" --type f --exec rg -l "ghc.?9\.10|ghc910" {}
# Look for any configuration or version constraints related to GHC 9.10
fd "\.nix$|\.cabal$|package\.yaml$" --type f --exec rg -l "ghc.?9\.10|ghc910" {}
Length of output: 1419
src/core/mkShellUtilityScripts.nix (2)
14-18: LGTM! Clean and well-structured functions
The formatting changes improve readability while maintaining the functional approach. The functions effectively handle script grouping with proper default values.
111-111: LGTM! Clean export declaration
The utility scripts are exported clearly and concisely.
src/lib/utils.nix (8)
20-20: LGTM: Formatting improvement
The indentation change aligns with Nix formatting standards and improves readability.
32-35: LGTM: Improved string concatenation readability
The multi-line format makes the complex string construction more readable and maintainable.
49-49: LGTM: Standard formatting
The formatting change aligns with Nix standards.
67-67: LGTM: Standard formatting
The formatting change aligns with Nix standards.
75-75: LGTM: Improved error message readability
The added empty lines enhance error message visibility in the terminal output.
Also applies to: 83-83
108-110: LGTM: Improved expression readability
The multi-line format makes the nested function calls more readable.
111-118: LGTM: Clearer option type structure
The formatting changes make the option type definition more readable.
121-121: LGTM: Standard formatting
The formatting change aligns with Nix standards.
src/core/mkHaskellProject.nix (5)
21-24: LGTM: Clean fallback logic for sphinxToolchain
The conditional assignment of sphinxToolchain is well-structured and provides a clear fallback mechanism.
37-37: LGTM: Proper duplicate checking before alias creation
The implementation correctly validates component names before creating aliases, preventing naming conflicts.
Also applies to: 46-47
73-73: LGTM: Well-structured shell profile configuration
Clean separation of concerns between read-the-docs and cabal project profiles, with proper tools configuration.
Also applies to: 77-84
117-129: LGTM: Well-organized project outputs
Clean implementation of flake outputs with proper integration of documentation components.
Line range hint 153-175: LGTM: Proper conditional job configuration
The implementation correctly handles conditional inclusion of different job types and combines them appropriately.
src/options/flake-dot-nix.nix (1)
230-230: LGTM! Formatting change from nixfmt-classic.
The formatting change aligns with the PR objective of replacing nixpkgs-fmt with nixfmt-classic.
Let's verify the formatting is consistent with other Nix files:
✅ Verification successful
Formatting is consistent with nixfmt-classic across the codebase
The formatting in flake-dot-nix.nix follows the same pattern as other Nix files in the codebase, where in { appears on a new line after a closing brace. The only difference is that this particular file has an inline assignment "flake.nix" = flake-dot-nix; within the same line, which is also a valid and clean formatting style for single-line declarations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other Nix files have similar formatting patterns
# Look for similar patterns in other Nix files to confirm consistent formatting
rg -t nix "^in \{"
Length of output: 227
Script:
#!/bin/bash
# Let's check the content around these 'in {' patterns to verify formatting consistency
rg -t nix -B 2 -A 2 "^in \{"
Length of output: 1105
src/core/mkShellWith.nix (8)
25-33: LGTM! Improved formatting of prompt and welcome message configuration.
The reformatting enhances readability while maintaining the same functionality.
36-39: LGTM! Improved formatting of GHC version configuration.
The conditional assignment is now more consistently formatted.
71-74: LGTM! Consistent tool configuration updates.
The getTool function formatting and tool list updates maintain consistency with the nixfmt-classic migration.
Also applies to: 86-86
131-146: LGTM! Pre-commit hooks are properly configured.
The hook configurations are consistently formatted and the nixfmt-classic hook is properly integrated.
Line range hint 156-195: LGTM! Improved formatting of hook configurations and toolchain profile.
The restructuring enhances readability while maintaining the same functionality.
225-273: LGTM! Enhanced profile and shell configuration formatting.
The reorganization improves code structure while maintaining functionality.
99-107: Track the TODO comment regarding nixfmt-classic naming conflicts.
The code includes a workaround for nixfmt-classic naming conflicts. This should be tracked and resolved once nixfmt-classic resolves the naming conflicts.
Would you like me to create a GitHub issue to track this TODO?
#!/bin/bash
# Check if nixfmt-classic has both 'nixfmt' and 'nixfmt-classic' executables
which nixfmt
which nixfmt-classic47-50: LGTM! Tool declarations are properly updated and formatted.
The change from nixpkgs-fmt to nixfmt-classic aligns with the PR objectives, and the formatting improvements enhance readability.
Also applies to: 55-56, 64-64
✅ Verification successful
The previous search might have failed due to case sensitivity or partial matches. Let's try a broader search to verify the replacement.
The replacement of nixpkgs-fmt with nixfmt-classic is properly implemented
The search results confirm that nixpkgs-fmt has been completely replaced with nixfmt-classic in the active codebase. The only remaining references to nixpkgs-fmt are in:
CHANGELOG.md: Documenting the migrationdoc/01-adr-iogx-interface.md: Historical architecture decision record
All functional code now uses nixfmt-classic, and the change is properly documented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that nixpkgs-fmt is completely replaced with nixfmt-classic
rg -l 'nixpkgs-fmt' .
Length of output: 183
Script:
#!/bin/bash
# Search for both old and new formatter references
rg -i "nixpkgs-fmt|nixfmt-classic" .
Length of output: 2323
src/options/mkFlake.nix (4)
19-21: Documentation formatting improvements look good!
The documentation strings have been consistently reformatted to improve readability by placing links on separate lines. The changes maintain a uniform style throughout the file.
Also applies to: 43-45, 79-81, 155-157, 181-183, 213-215, 219-221, 244-246
34-39: Systems option formatting improvements look good!
The vertical listing of architectures and aligned default values improve readability while maintaining the same functionality.
Also applies to: 47-50
166-172: Documentation content updates are valuable!
The additions provide helpful examples for inputs usage and clearly document available library functions, improving the module's usability.
Also applies to: 196-204
321-321: Module structure formatting looks good!
The export section formatting maintains clean and consistent style.
src/options/mkHaskellProject.nix (3)
120-129: Documentation improvements look good!
The documentation updates enhance readability while maintaining semantic meaning. The changes include:
- Better formatting of cross-references using the
linkfunction - Clearer explanations of options and their relationships
- Improved structure of documentation blocks
Also applies to: 280-282, 318-320, 354-356, 362-364, 367-369, 376-399, 463-465, 472-474, 481-483, 490-492, 499-501, 508-510, 518-520
127-129: Link formatting is consistent and well-structured!
The use of the link function for cross-references is consistent throughout the file, improving maintainability and readability.
Also applies to: 280-282, 318-320, 354-356, 362-364, 367-369, 376-399, 463-465, 472-474, 481-483, 490-492, 499-501, 508-510, 518-520
604-605: Type declaration formatting improvement looks good!
The split of the type declaration across lines enhances readability while maintaining the same functionality.
src/options/mkShell.nix (6)
18-32: LGTM: Documentation improvements enhance clarity
The added clarification about valid compiler versions and improved formatting make the documentation more user-friendly.
Line range hint 51-186: LGTM: Consistent link formatting
The link formatting has been standardized using string interpolation, which improves maintainability and readability.
Line range hint 417-455: LGTM: Documentation formatting improvements
The link formatting improvements enhance readability while maintaining the same functionality.
477-479: LGTM: Consistent link formatting in descriptions
The link formatting changes maintain consistency with the rest of the file.
Also applies to: 568-570, 718-720
589-597: LGTM: Pre-commit hooks updated consistently
The pre-commit hooks section has been updated to use the new nixfmt-classic name consistently.
Also applies to: 638-639
248-260: LGTM: Tool renamed from nixpkgs-fmt to nixfmt-classic
The renaming is consistent with the PR objectives.
Let's verify the consistency of the renaming across the codebase:
doc/api.md (3)
1653-1654: LGTM: nixfmt-classic preCommit configuration is well documented.
The documentation for the nixfmt-classic preCommit hook follows the established pattern and provides the necessary configuration options.
1706-1706: LGTM: nixfmt-classic is properly listed as a pre-configured hook.
The nixfmt-classic hook is correctly added to the list of pre-configured hooks.
Line range hint 2191-2209: LGTM: nixfmt-classic tool configuration is well documented.
The documentation for the nixfmt-classic tool configuration is complete and follows the established pattern, including all necessary fields (type, default, example, and description).
| scripts = let | ||
| scripts1 = utils.getAttrWithDefault "scripts" { } p1; | ||
| scripts2 = utils.getAttrWithDefault "scripts" { } p2; | ||
| # TODO check clashes | ||
| scripts1 // scripts2; | ||
| in scripts1 // scripts2; |
There was a problem hiding this comment.
Consider implementing clash detection for script merging.
The current implementation silently overrides scripts from p1 with those from p2 when there are naming conflicts. This could lead to unexpected behavior.
Consider implementing one of these approaches:
- Throw an error when detecting script name conflicts
- Add a prefix/suffix to conflicting script names
- Implement a merge strategy that combines conflicting scripts
Here's a suggested implementation for approach #1:
scripts = let
scripts1 = utils.getAttrWithDefault "scripts" { } p1;
scripts2 = utils.getAttrWithDefault "scripts" { } p2;
commonKeys = builtins.intersectAttrs scripts1 scripts2;
hasClashes = (builtins.length (builtins.attrNames commonKeys)) > 0;
in if hasClashes
then throw "Script name conflicts detected: ${toString (builtins.attrNames commonKeys)}"
else scripts1 // scripts2;| env = let | ||
| env1 = utils.getAttrWithDefault "env" { } p1; | ||
| env2 = utils.getAttrWithDefault "env" { } p2; | ||
| # TODO check clashes | ||
| env1 // env2; | ||
|
|
||
| shellHook = | ||
| lib.concatStringsSep "\n" [ | ||
| (utils.getAttrWithDefault "shellHook" "" p1) | ||
| (utils.getAttrWithDefault "shellHook" "" p2) | ||
| ]; | ||
|
|
||
| tools = | ||
| let | ||
| tools1 = utils.getAttrWithDefault "tools" { } p1; | ||
| tools2 = utils.getAttrWithDefault "tools" { } p2; | ||
| in | ||
| in env1 // env2; |
There was a problem hiding this comment.
Consider implementing clash detection for environment variables.
Similar to scripts, environment variables from p1 are silently overridden by those from p2 during conflicts. This could affect build reproducibility.
Consider implementing clash detection similar to the scripts suggestion above, or implement a merge strategy that combines values (e.g., for PATH-like variables).
Example implementation for PATH-like variables:
env = let
env1 = utils.getAttrWithDefault "env" { } p1;
env2 = utils.getAttrWithDefault "env" { } p2;
mergeEnvVar = name: v1: v2:
if lib.hasPrefix "PATH" name
then "${v1}:${v2}"
else throw "Environment variable conflict: ${name}";
merged = lib.zipAttrsWith mergeEnvVar [ env1 env2 ];
in merged;
Summary by CodeRabbit
Release Notes
New Features
nixpkgs-fmttonixfmt-classic.mkShell,mkHaskellProject, andmkContainerFromCabalExe.Bug Fixes
Documentation
Refactor