Skip to content

Conversation

@edolstra
Copy link
Collaborator

@edolstra edolstra commented Oct 23, 2025

Motivation

Changes

git+file:///home/eelco/Dev/nix
├───checks
│   ├───aarch64-darwin
│   │   ├───installerScriptForGHA: omitted 'omitted (use '--all-systems' to show)'
...
        └───nix-util-tests-stdenv: derivation 'package 'nix-util-tests-3.11.3''

to

git+file:///home/eelco/Dev/nix
├───checks
│   ├───aarch64-darwin
│   │   ├───installerScriptForGHA: omitted (use '--all-systems' to show)
...
        └───nix-util-tests-stdenv: package 'nix-util-tests-3.11.3'

Also includes a simplification to LocalStore::addToStore().

Context

Summary by CodeRabbit

  • Style

    • Streamlined flake output formatting to display headers more concisely, removing verbose type/details from the rendered output.
  • Refactor

    • Simplified internal store cleanup behavior to make cleanup more consistent and robust, reducing unexpected fallback paths.

@coderabbitai
Copy link

coderabbitai bot commented Oct 23, 2025

Walkthrough

Cleanup in LocalStore::addToStore now always attempts to skip narSize and silence exceptions instead of parsing a dump; flake output formatting was simplified to print only the header and resolved string.

Changes

Cohort / File(s) Summary
NAR data cleanup simplification
src/libstore/local-store.cc
Removed conditional dump-parsing fallback and related exception handling; cleanup now consistently calls source.skip(info.narSize) inside a try/catch and ignores exceptions when narRead is false.
Output formatting update
src/nix/flake.cc
Changed logger output for rendered JSON/header to print only headerPrefix and the resolved string s, removing explicit type and quoted value from the message.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant LocalStore
    participant Source

    Caller->>LocalStore: addToStore(...)
    alt previous flow (before)
        LocalStore->>Source: attempt read (narRead false)
        Note right of LocalStore: tried parseDump fallback\nwith NullFileSystemObjectSink
        Source-->>LocalStore: read failure / skip not used
        LocalStore->>LocalStore: parseDump(...) as fallback
    else new flow (after)
        LocalStore->>Source: call source.skip(narSize) within try
        Source-->>LocalStore: may throw (ignored)
        LocalStore->>Caller: cleanup complete
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • cole-h

Poem

🐰 I hop through bytes the gentle way,
No fallback parsing clouds my day.
Headers trimmed to just what's true,
A lighter log and cleaner roo—
Thump, thump, I nibble bugs away.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "nix flake check: Fix output rendering" directly and clearly describes the primary change in this changeset. The main modification in src/nix/flake.cc addresses output formatting by removing explicit type information and quoted values from logger output, which is exactly what "Fix output rendering" conveys. The title is specific, concise, and avoids vague or generic language. While there is a secondary change involving LocalStore::addToStore cleanup in src/libstore/local-store.cc, the instructions acknowledge that titles are not required to cover every detail of the changeset, and focusing on the primary user-facing change is appropriate.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-flake-show-output

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 35a8e60 and 15efebd.

📒 Files selected for processing (1)
  • src/libstore/local-store.cc (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/libstore/local-store.cc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build_x86_64-linux / build
  • GitHub Check: build_aarch64-darwin / build

Comment @coderabbitai help to get the list of available commands and usage tips.

Changes
```
git+file:///home/eelco/Dev/nix
├───checks
│   ├───aarch64-darwin
│   │   ├───installerScriptForGHA: omitted 'omitted (use '--all-systems' to show)'
...
        └───nix-util-tests-stdenv: derivation 'package 'nix-util-tests-3.11.3''
```
to
```
git+file:///home/eelco/Dev/nix
├───checks
│   ├───aarch64-darwin
│   │   ├───installerScriptForGHA: omitted (use '--all-systems' to show)
...
        └───nix-util-tests-stdenv: package 'nix-util-tests-3.11.3'
```
@edolstra edolstra force-pushed the fix-flake-show-output branch from f408f3b to 35a8e60 Compare October 23, 2025 14:28
@edolstra edolstra enabled auto-merge October 23, 2025 14:28
@github-actions
Copy link

github-actions bot commented Oct 23, 2025

@github-actions github-actions bot temporarily deployed to pull request October 23, 2025 14:32 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 60db4b9 and 35a8e60.

📒 Files selected for processing (2)
  • src/libstore/local-store.cc (1 hunks)
  • src/nix/flake.cc (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/libstore/local-store.cc (1)
src/libstore/dummy-store.cc (4)
  • source (186-250)
  • source (186-193)
  • info (160-184)
  • info (160-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build_aarch64-darwin / build
  • GitHub Check: build_x86_64-linux / build
🔇 Additional comments (1)
src/nix/flake.cc (1)

1385-1385: LGTM! Cleaner output format.

The simplified format removes redundant type prefixes and extra quotes. The variable s already contains the formatted string with appropriate context (e.g., "package 'name'" for derivations, full message for omitted items).

`skip()` can throw an exception, which we need to ignore since we may
be unwinding an exception.
@edolstra edolstra disabled auto-merge October 23, 2025 14:36
@edolstra edolstra enabled auto-merge October 23, 2025 14:38
@github-actions github-actions bot temporarily deployed to pull request October 23, 2025 14:42 Inactive
@edolstra edolstra added this pull request to the merge queue Oct 23, 2025
Merged via the queue into main with commit 85b66c9 Oct 23, 2025
35 checks passed
@edolstra edolstra deleted the fix-flake-show-output branch October 23, 2025 15:17
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.

2 participants