Skip to content

Conversation

@dyegoaurelio
Copy link
Contributor

I noticed this while working on #353

list rendering code inside function arguments has special handling and was not considering language annotations.

This ensures the language annotations are formatted as intended on this case.

Before:

  runScripts = (
    lib.mkSomething
      [
        /* bash */
        ''
          echo "Script A"
        ''
      ]
      [
        /* python */
        ''
          print("Script B")
        ''
        /* ruby */
        "puts 'Script C'"
      ]
  );

Now:

  runScripts = (
    lib.mkSomething
      [
        /* bash */ ''
          echo "Script A"
        ''
      ]
      [
        /* python */ ''
          print("Script B")
        ''
        /* ruby */ "puts 'Script C'"
      ]
  );

@github-actions
Copy link

github-actions bot commented Nov 8, 2025

Nixpkgs diff

@piegamesde
Copy link
Member

Honestly, this feels like a bandaid. If the original implementation of the feature requires such fixes, I'd suggest revising its fundamental approach rather than going through all edge cases of the code base one by one over the years.

@dyegoaurelio
Copy link
Contributor Author

dyegoaurelio commented Nov 9, 2025

Honestly, this feels like a bandaid. If the original implementation of the feature requires such fixes, I'd suggest revising its fundamental approach rather than going through all edge cases of the code base one by one over the years.

It does, but I'm not sure how we could refactor the implementation to avoid edge cases like this one.

The formatting for the language annotation comment is different from the formatting for other comments on list entries.

So I added explicit handling for it on the list item rendering code

 -- For lists, attribute sets and let bindings
 prettyItems :: (Pretty a) => Items a -> Doc
-prettyItems (Items items) = sepBy hardline items
+prettyItems (Items items) = go items
+  where
+    go [] = mempty
+    go [item] = pretty item
+    -- Special case: language annotation comment followed by string item
+    go (Comments [LanguageAnnotation lang] : Item stringItem : rest) =
+      pretty (LanguageAnnotation lang)
+        <> hardspace
+        <> group stringItem
+        <> if null rest then mempty else hardline <> go rest
+    go (item : rest) =
+      pretty item <> if null rest then mempty else hardline <> go rest

I just wasn't aware that there was another code path that was formatting lists without calling this function.

Maybe a nice approach here would be to make the list rendering code a bit more DRY, by creating a function that's called by both absorbInner and prettyTerm (List paropen@Ann{trailComment = post} items parclose)

@dyegoaurelio dyegoaurelio force-pushed the lang-annotation-in-list-argument branch 2 times, most recently from af44960 to 951ba18 Compare November 24, 2025 00:47
List rendering code inside function arguments (`absorbInner`) has special
handling and was not considering language annotations, causing them to be
placed on a separate line from the string they annotate.

To prevent similar bugs, the list rendering code is now DRY:
- Rename `prettyItems` to `renderItems` with a configurable item separator
- Extract `renderList` helper used by both `prettyTerm` and `absorbInner`

Language annotation handling is now consistent across all list rendering code paths.

Before:
```nix
  runScripts = (
    lib.mkSomething
      [
        /* bash */
        ''
          echo "Script A"
        ''
      ]
  );
```

After:
```nix
  runScripts = (
    lib.mkSomething
      [
        /* bash */ ''
          echo "Script A"
        ''
      ]
  );
```
@dyegoaurelio dyegoaurelio force-pushed the lang-annotation-in-list-argument branch from 951ba18 to f4bbd8c Compare November 24, 2025 00:51
Copy link
Collaborator

@jfly jfly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as a team. LGTM!

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dyegoaurelio dyegoaurelio merged commit 42e43d9 into NixOS:master Nov 25, 2025
2 checks passed
@dyegoaurelio dyegoaurelio deleted the lang-annotation-in-list-argument branch November 25, 2025 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants