From 331e0aa71f789b704ab7c8564ec5328a9186d024 Mon Sep 17 00:00:00 2001 From: piegames Date: Sun, 11 Aug 2024 12:37:05 +0200 Subject: [PATCH 1/2] Don't expand lists within functions --- src/Nixfmt/Pretty.hs | 25 +++++++-- src/Nixfmt/Types.hs | 2 +- test/diff/apply/in.nix | 2 +- test/diff/apply/out-pure.nix | 16 ++---- test/diff/apply/out.nix | 2 +- test/diff/idioms_lib_3/out-pure.nix | 30 +++-------- test/diff/idioms_lib_3/out.nix | 30 +++-------- test/diff/idioms_nixos_2/out-pure.nix | 73 ++++++++------------------- test/diff/idioms_nixos_2/out.nix | 73 ++++++++------------------- 9 files changed, 84 insertions(+), 169 deletions(-) diff --git a/src/Nixfmt/Pretty.hs b/src/Nixfmt/Pretty.hs index 83b02afb..bcda8a94 100644 --- a/src/Nixfmt/Pretty.hs +++ b/src/Nixfmt/Pretty.hs @@ -375,15 +375,34 @@ instance Pretty Parameter where -- then start on a new line instead". prettyApp :: Bool -> Doc -> Bool -> Expression -> Expression -> Doc prettyApp indentFunction pre hasPost f a = - let -- This is very hacky, but selections shouldn't be in a priority group, + let -- Walk the function call chain + absorbApp :: Expression -> Doc + -- This is very hacky, but selections shouldn't be in a priority group, -- because if they get expanded before anything else, -- only the `.`-and-after part gets to a new line, which looks very odd - absorbApp (Application f' a'@(Term Selection{})) = group' Transparent (absorbApp f') <> line <> nest (group' RegularG a') - absorbApp (Application f' a') = group' Transparent (absorbApp f') <> line <> nest (group' Priority a') + absorbApp (Application f' a'@(Term Selection{})) = group' Transparent (absorbApp f') <> line <> nest (group' RegularG $ absorbInner a') + absorbApp (Application f' a') = group' Transparent (absorbApp f') <> line <> nest (group' Priority $ absorbInner a') + -- First argument absorbApp expr | indentFunction && null comment' = nest $ group' RegularG $ line' <> pretty expr | otherwise = pretty expr + -- Render the inner arguments of a function call + absorbInner :: Expression -> Doc + -- If lists have only simple items, try to render them single-line instead of expanding + -- This is just a copy of the list rendering code, but with `sepBy line` instead of `sepBy hardline` + absorbInner (Term (List paropen@Ann{trailComment = post'} items parclose)) + | length (unItems items) <= 4 && all (isSimple . Term) items = + pretty (paropen{trailComment = Nothing}) + <> surroundWith sur (nest $ pretty post' <> sepBy line (unItems items)) + <> pretty parclose + where + -- If the brackets are on different lines, keep them like that + sur = if sourceLine paropen /= sourceLine parclose then hardline else line + absorbInner expr = pretty expr + + -- Render the last argument of a function call + absorbLast :: Expression -> Doc absorbLast (Term t) | isAbsorbable t = group' Priority $ nest $ prettyTerm t diff --git a/src/Nixfmt/Types.hs b/src/Nixfmt/Types.hs index 262a4acb..93dd1d02 100644 --- a/src/Nixfmt/Types.hs +++ b/src/Nixfmt/Types.hs @@ -122,7 +122,7 @@ data Item a Comments Trivia deriving (Foldable, Show, Functor) -newtype Items a = Items {unItems :: [Item a]} deriving (Functor) +newtype Items a = Items {unItems :: [Item a]} deriving (Functor, Foldable) instance (Eq a) => Eq (Items a) where (==) = (==) `on` concatMap Data.Foldable.toList . unItems diff --git a/test/diff/apply/in.nix b/test/diff/apply/in.nix index d8c22eb0..0a639697 100644 --- a/test/diff/apply/in.nix +++ b/test/diff/apply/in.nix @@ -65,7 +65,7 @@ mapAttrsToStringsSep "\n" mkSection attrsOfAttrs; } [ - (mapAttrsToStringsSep [force long] "\n" mkSection attrsOfAttrs) + (mapAttrsToStringsSep [force /* meow */ long] "\n" mkSection attrsOfAttrs) ] (a b) diff --git a/test/diff/apply/out-pure.nix b/test/diff/apply/out-pure.nix index 42497b7f..99114616 100644 --- a/test/diff/apply/out-pure.nix +++ b/test/diff/apply/out-pure.nix @@ -71,7 +71,7 @@ } [ (mapAttrsToStringsSep [ - force + force # meow long ] "\n" mkSection attrsOfAttrs) ] @@ -160,16 +160,10 @@ ''"'' "\${" ]; - escapeMultiline = - libStr.replaceStrings - [ - "\${" - "''" - ] - [ - "''\${" - "'''" - ]; + escapeMultiline = libStr.replaceStrings [ "\${" "''" ] [ + "''\${" + "'''" + ]; test = foo [ diff --git a/test/diff/apply/out.nix b/test/diff/apply/out.nix index a30e59ad..6692fd4e 100644 --- a/test/diff/apply/out.nix +++ b/test/diff/apply/out.nix @@ -71,7 +71,7 @@ } [ (mapAttrsToStringsSep [ - force + force # meow long ] "\n" mkSection attrsOfAttrs) ] diff --git a/test/diff/idioms_lib_3/out-pure.nix b/test/diff/idioms_lib_3/out-pure.nix index 1115ae8b..387e9df3 100644 --- a/test/diff/idioms_lib_3/out-pure.nix +++ b/test/diff/idioms_lib_3/out-pure.nix @@ -130,13 +130,7 @@ rec { toINI = { # apply transformations (e.g. escapes) to section names - mkSectionName ? ( - name: - libStr.escape [ - "[" - "]" - ] name - ), + mkSectionName ? (name: libStr.escape [ "[" "]" ] name), # format a setting line from key and value mkKeyValue ? mkKeyValueDefault { } "=", # allow lists as values for duplicate keys @@ -191,13 +185,7 @@ rec { toINIWithGlobalSection = { # apply transformations (e.g. escapes) to section names - mkSectionName ? ( - name: - libStr.escape [ - "[" - "]" - ] name - ), + mkSectionName ? (name: libStr.escape [ "[" "]" ] name), # format a setting line from key and value mkKeyValue ? mkKeyValueDefault { } "=", # allow lists as values for duplicate keys @@ -378,16 +366,10 @@ rec { ''"'' "\${" ]; - escapeMultiline = - libStr.replaceStrings - [ - "\${" - "''" - ] - [ - "''\${" - "'''" - ]; + escapeMultiline = libStr.replaceStrings [ "\${" "''" ] [ + "''\${" + "'''" + ]; singlelineResult = ''"'' + concatStringsSep "\\n" (map escapeSingleline lines) + ''"''; multilineResult = diff --git a/test/diff/idioms_lib_3/out.nix b/test/diff/idioms_lib_3/out.nix index 4a1b2843..469184e6 100644 --- a/test/diff/idioms_lib_3/out.nix +++ b/test/diff/idioms_lib_3/out.nix @@ -133,13 +133,7 @@ rec { toINI = { # apply transformations (e.g. escapes) to section names - mkSectionName ? ( - name: - libStr.escape [ - "[" - "]" - ] name - ), + mkSectionName ? (name: libStr.escape [ "[" "]" ] name), # format a setting line from key and value mkKeyValue ? mkKeyValueDefault { } "=", # allow lists as values for duplicate keys @@ -194,13 +188,7 @@ rec { toINIWithGlobalSection = { # apply transformations (e.g. escapes) to section names - mkSectionName ? ( - name: - libStr.escape [ - "[" - "]" - ] name - ), + mkSectionName ? (name: libStr.escape [ "[" "]" ] name), # format a setting line from key and value mkKeyValue ? mkKeyValueDefault { } "=", # allow lists as values for duplicate keys @@ -391,16 +379,10 @@ rec { ''"'' "\${" ]; - escapeMultiline = - libStr.replaceStrings - [ - "\${" - "''" - ] - [ - "''\${" - "'''" - ]; + escapeMultiline = libStr.replaceStrings [ "\${" "''" ] [ + "''\${" + "'''" + ]; singlelineResult = ''"'' + concatStringsSep "\\n" (map escapeSingleline lines) + ''"''; multilineResult = diff --git a/test/diff/idioms_nixos_2/out-pure.nix b/test/diff/idioms_nixos_2/out-pure.nix index e7d3e5d2..a2743768 100644 --- a/test/diff/idioms_nixos_2/out-pure.nix +++ b/test/diff/idioms_nixos_2/out-pure.nix @@ -72,58 +72,27 @@ in { imports = [ - (mkRemovedOptionModule - [ - "services" - "nextcloud" - "config" - "adminpass" - ] - '' - Please use `services.nextcloud.config.adminpassFile' instead! - '' - ) - (mkRemovedOptionModule - [ - "services" - "nextcloud" - "config" - "dbpass" - ] - '' - Please use `services.nextcloud.config.dbpassFile' instead! - '' - ) - (mkRemovedOptionModule - [ - "services" - "nextcloud" - "nginx" - "enable" - ] - '' - The nextcloud module supports `nginx` as reverse-proxy by default and doesn't - support other reverse-proxies officially. - - However it's possible to use an alternative reverse-proxy by - - * disabling nginx - * setting `listen.owner` & `listen.group` in the phpfpm-pool to a different value - - Further details about this can be found in the `Nextcloud`-section of the NixOS-manual - (which can be opened e.g. by running `nixos-help`). - '' - ) - (mkRemovedOptionModule - [ - "services" - "nextcloud" - "disableImagemagick" - ] - '' - Use services.nextcloud.enableImagemagick instead. - '' - ) + (mkRemovedOptionModule [ "services" "nextcloud" "config" "adminpass" ] '' + Please use `services.nextcloud.config.adminpassFile' instead! + '') + (mkRemovedOptionModule [ "services" "nextcloud" "config" "dbpass" ] '' + Please use `services.nextcloud.config.dbpassFile' instead! + '') + (mkRemovedOptionModule [ "services" "nextcloud" "nginx" "enable" ] '' + The nextcloud module supports `nginx` as reverse-proxy by default and doesn't + support other reverse-proxies officially. + + However it's possible to use an alternative reverse-proxy by + + * disabling nginx + * setting `listen.owner` & `listen.group` in the phpfpm-pool to a different value + + Further details about this can be found in the `Nextcloud`-section of the NixOS-manual + (which can be opened e.g. by running `nixos-help`). + '') + (mkRemovedOptionModule [ "services" "nextcloud" "disableImagemagick" ] '' + Use services.nextcloud.enableImagemagick instead. + '') ]; options.services.nextcloud = { diff --git a/test/diff/idioms_nixos_2/out.nix b/test/diff/idioms_nixos_2/out.nix index 7502b9f9..b63a9379 100644 --- a/test/diff/idioms_nixos_2/out.nix +++ b/test/diff/idioms_nixos_2/out.nix @@ -75,58 +75,27 @@ in { imports = [ - (mkRemovedOptionModule - [ - "services" - "nextcloud" - "config" - "adminpass" - ] - '' - Please use `services.nextcloud.config.adminpassFile' instead! - '' - ) - (mkRemovedOptionModule - [ - "services" - "nextcloud" - "config" - "dbpass" - ] - '' - Please use `services.nextcloud.config.dbpassFile' instead! - '' - ) - (mkRemovedOptionModule - [ - "services" - "nextcloud" - "nginx" - "enable" - ] - '' - The nextcloud module supports `nginx` as reverse-proxy by default and doesn't - support other reverse-proxies officially. - - However it's possible to use an alternative reverse-proxy by - - * disabling nginx - * setting `listen.owner` & `listen.group` in the phpfpm-pool to a different value - - Further details about this can be found in the `Nextcloud`-section of the NixOS-manual - (which can be opened e.g. by running `nixos-help`). - '' - ) - (mkRemovedOptionModule - [ - "services" - "nextcloud" - "disableImagemagick" - ] - '' - Use services.nextcloud.enableImagemagick instead. - '' - ) + (mkRemovedOptionModule [ "services" "nextcloud" "config" "adminpass" ] '' + Please use `services.nextcloud.config.adminpassFile' instead! + '') + (mkRemovedOptionModule [ "services" "nextcloud" "config" "dbpass" ] '' + Please use `services.nextcloud.config.dbpassFile' instead! + '') + (mkRemovedOptionModule [ "services" "nextcloud" "nginx" "enable" ] '' + The nextcloud module supports `nginx` as reverse-proxy by default and doesn't + support other reverse-proxies officially. + + However it's possible to use an alternative reverse-proxy by + + * disabling nginx + * setting `listen.owner` & `listen.group` in the phpfpm-pool to a different value + + Further details about this can be found in the `Nextcloud`-section of the NixOS-manual + (which can be opened e.g. by running `nixos-help`). + '') + (mkRemovedOptionModule [ "services" "nextcloud" "disableImagemagick" ] '' + Use services.nextcloud.enableImagemagick instead. + '') ]; options.services.nextcloud = { From d8caa4ca71530c98f07be9489dfcf89683de1d34 Mon Sep 17 00:00:00 2001 From: piegames Date: Sat, 17 Aug 2024 16:07:08 +0200 Subject: [PATCH 2/2] Don't force-expand attrsets that contain a single inherit --- src/Nixfmt/Pretty.hs | 6 +++++- test/diff/idioms_lib_4/out-pure.nix | 24 ++++++------------------ test/diff/idioms_lib_4/out.nix | 24 ++++++------------------ test/diff/idioms_nixos_1/out-pure.nix | 4 +--- test/diff/idioms_nixos_1/out.nix | 4 +--- 5 files changed, 19 insertions(+), 43 deletions(-) diff --git a/src/Nixfmt/Pretty.hs b/src/Nixfmt/Pretty.hs index bcda8a94..f03f7bcf 100644 --- a/src/Nixfmt/Pretty.hs +++ b/src/Nixfmt/Pretty.hs @@ -541,7 +541,11 @@ absorbExpr _ expr = pretty expr -- Render the RHS value of an assignment or function parameter default value absorbRHS :: Expression -> Doc absorbRHS expr = case expr of - -- Absorbable expression. Always start on the same line + -- Exception to the case below: Don't force-expand attrsets if they only contain a single inherit statement + (Term (Set _ _ binders _)) + | case unItems binders of [Item (Inherit{})] -> True; _ -> False -> + hardspace <> group (absorbExpr False expr) + -- Absorbable expression. Always start on the same line, and force-expand attrsets _ | isAbsorbableExpr expr -> hardspace <> group (absorbExpr True expr) -- Parenthesized expression. Same thing as the special case for parenthesized last argument in function calls. (Term (Parenthesized open expr' close)) -> hardspace <> absorbParen open expr' close diff --git a/test/diff/idioms_lib_4/out-pure.nix b/test/diff/idioms_lib_4/out-pure.nix index 8ff78dae..c1ed4245 100644 --- a/test/diff/idioms_lib_4/out-pure.nix +++ b/test/diff/idioms_lib_4/out-pure.nix @@ -521,33 +521,25 @@ rec { # the normalized name for macOS. macos = { execFormat = macho; - families = { - inherit darwin; - }; + families = { inherit darwin; }; name = "darwin"; }; ios = { execFormat = macho; - families = { - inherit darwin; - }; + families = { inherit darwin; }; }; # A tricky thing about FreeBSD is that there is no stable ABI across # versions. That means that putting in the version as part of the # config string is paramount. freebsd12 = { execFormat = elf; - families = { - inherit bsd; - }; + families = { inherit bsd; }; name = "freebsd"; version = 12; }; freebsd13 = { execFormat = elf; - families = { - inherit bsd; - }; + families = { inherit bsd; }; name = "freebsd"; version = 13; }; @@ -557,9 +549,7 @@ rec { }; netbsd = { execFormat = elf; - families = { - inherit bsd; - }; + families = { inherit bsd; }; }; none = { execFormat = unknown; @@ -567,9 +557,7 @@ rec { }; openbsd = { execFormat = elf; - families = { - inherit bsd; - }; + families = { inherit bsd; }; }; solaris = { execFormat = elf; diff --git a/test/diff/idioms_lib_4/out.nix b/test/diff/idioms_lib_4/out.nix index 8ff78dae..c1ed4245 100644 --- a/test/diff/idioms_lib_4/out.nix +++ b/test/diff/idioms_lib_4/out.nix @@ -521,33 +521,25 @@ rec { # the normalized name for macOS. macos = { execFormat = macho; - families = { - inherit darwin; - }; + families = { inherit darwin; }; name = "darwin"; }; ios = { execFormat = macho; - families = { - inherit darwin; - }; + families = { inherit darwin; }; }; # A tricky thing about FreeBSD is that there is no stable ABI across # versions. That means that putting in the version as part of the # config string is paramount. freebsd12 = { execFormat = elf; - families = { - inherit bsd; - }; + families = { inherit bsd; }; name = "freebsd"; version = 12; }; freebsd13 = { execFormat = elf; - families = { - inherit bsd; - }; + families = { inherit bsd; }; name = "freebsd"; version = 13; }; @@ -557,9 +549,7 @@ rec { }; netbsd = { execFormat = elf; - families = { - inherit bsd; - }; + families = { inherit bsd; }; }; none = { execFormat = unknown; @@ -567,9 +557,7 @@ rec { }; openbsd = { execFormat = elf; - families = { - inherit bsd; - }; + families = { inherit bsd; }; }; solaris = { execFormat = elf; diff --git a/test/diff/idioms_nixos_1/out-pure.nix b/test/diff/idioms_nixos_1/out-pure.nix index f2bcf7ff..f8ec98ac 100644 --- a/test/diff/idioms_nixos_1/out-pure.nix +++ b/test/diff/idioms_nixos_1/out-pure.nix @@ -284,9 +284,7 @@ in }) (mkIf (!config.boot.isContainer) { - system.build = { - inherit kernel; - }; + system.build = { inherit kernel; }; system.modulesTree = [ kernel ] ++ config.boot.extraModulePackages; diff --git a/test/diff/idioms_nixos_1/out.nix b/test/diff/idioms_nixos_1/out.nix index f2bcf7ff..f8ec98ac 100644 --- a/test/diff/idioms_nixos_1/out.nix +++ b/test/diff/idioms_nixos_1/out.nix @@ -284,9 +284,7 @@ in }) (mkIf (!config.boot.isContainer) { - system.build = { - inherit kernel; - }; + system.build = { inherit kernel; }; system.modulesTree = [ kernel ] ++ config.boot.extraModulePackages;