Skip to content

Conversation

z-Fng
Copy link

@z-Fng z-Fng commented Jul 12, 2025

Description

This PR makes the following changes:

  • feat(install): Add separator at the end of notes, highlight suggestions.

Motivation and Context

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

Summary by CodeRabbit

  • New Features

    • Installer now adds a separator after displayed notes for clearer readability.
    • Unmet feature suggestions are highlighted in DarkYellow for improved visibility.
  • Documentation

    • Updated changelog under Unreleased to reflect the new installer display enhancements.

@z-Fng z-Fng force-pushed the opt-scoop-install branch from 1b7b8fc to 25ca8a1 Compare August 13, 2025 20:40
@HUMORCE
Copy link
Member

HUMORCE commented Aug 13, 2025

The separator should only output when suggestion existed instead always, if we have to do this.

@z-Fng
Copy link
Author

z-Fng commented Aug 15, 2025

Maybe my title is a bit misleading. I think the separator at the end of notes should always be displayed if possible.

This is because it not only separates notes from suggestions, but also separates notes from the output (or warnings) of other apps installed at the same time.

Here are some examples:

  • Single app with only notes: scoop install 1password-cli
    • Before: image
    • After: image
  • Single app with both notes and suggestions: scoop install 1password-cli
    • Before: image
    • After: image
  • Multiple apps: scoop install bind btop-lhm
    • Before: image
    • After: image

The above are just my suggestions. (: There may be better ways to separate them.

The separator should only output when suggestion existed instead always, if we have to do this.

For the situation with only notes, it is also acceptable, right?
image

@z-Fng z-Fng changed the title feat(install): Add separation between suggestions and notes, highlight suggestions feat(install): Add separation at the end of notes, highlight suggestions Aug 15, 2025
@HUMORCE
Copy link
Member

HUMORCE commented Aug 18, 2025

We could remove Notes character, when it are wrapped by separator. In some cases, it does not a 'note'.

@z-Fng
Copy link
Author

z-Fng commented Aug 18, 2025

We could remove Notes character, when it are wrapped by separator. In some cases, it does not a 'note'.

That makes sense to me. Simply highlighting suggestions would be helpful as well.

Copy link

coderabbitai bot commented Sep 24, 2025

Walkthrough

Added output formatting tweaks in the install script: an extra separator after notes and DarkYellow highlighting for unmet feature suggestions. Updated CHANGELOG under Unreleased to document this feature.

Changes

Cohort / File(s) Summary
Documentation
CHANGELOG.md
Added Unreleased feature entry: “install: Add separator at the end of notes, highlight suggestions (#6418)”.
Install output formatting
lib/install.ps1
In show_notes: prints an additional separator after notes when present. Changes Write-Host color to DarkYellow for unfulfilled feature suggestions. No logic changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I hopped through code with paws so light,
Drew lines of dashes, just right-bright,
Notes now bookended, neat and clear—
Suggestions glow, in amber cheer.
With whiskers twitching, I commit:
A tiny shine—UI well-lit.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately summarizes the primary change — adding a separator after notes and highlighting suggestions — and is concise, relevant, and directly reflective of the changes in lib/install.ps1 and CHANGELOG.md.
Linked Issues Check ✅ Passed The PR implements the two coding objectives from issue [#6417]: it adds a separator at the end of the notes block (show_notes now emits an extra separator when manifest.notes exists) and highlights suggestion output by coloring suggestion messages (DarkYellow), and the changelog entry was updated to reflect the feature, so the requested visual-separation and highlighting goals are met.
Out of Scope Changes Check ✅ Passed All modifications are limited to lib/install.ps1 and CHANGELOG.md and are directly related to the requested UI changes (separator after notes and colored suggestion output); there are no unrelated file changes, public API alterations, or other out-of-scope code edits in the provided summary.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@z-Fng z-Fng changed the title feat(install): Add separation at the end of notes, highlight suggestions feat(install): Add separator at the end of notes, highlight suggestions Sep 24, 2025
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: 0

🧹 Nitpick comments (3)
CHANGELOG.md (1)

3-6: Wording nit: tighten the feature bullet

Consider slight wording polish for consistency with other entries.

Apply this diff:

-- **install:** Add separator at the end of notes, highlight suggestions ([#6418](https://github.com/ScoopInstaller/Scoop/issues/6418))
+- **install:** Add separator after notes; highlight suggestions ([#6418](https://github.com/ScoopInstaller/Scoop/issues/6418))
lib/install.ps1 (2)

358-364: Optional: drop the “Notes” label when wrapped by separators

Per review thread, removing the header reduces redundancy when the block is delimited above and below.

Apply this diff:

 function show_notes($manifest, $dir, $original_dir, $persist_dir) {
     if ($manifest.notes) {
-        Write-Output 'Notes'
         Write-Output '-----'
         Write-Output (wraptext (substitute $manifest.notes @{ '$dir' = $dir; '$original_dir' = $original_dir; '$persist_dir' = $persist_dir }))
         Write-Output '-----'
     }
 }

422-424: Color choice: avoid conflating with warnings

If warn logs are yellow in this codebase, DarkYellow suggestions may read as warnings. Consider a non-warning color (e.g., Cyan or DarkCyan) to distinguish suggestions.

Would you like to switch to Cyan? If so:

-                Write-Host "'$app' suggests installing '$([string]::join("' or '", $feature_suggestions))'." -ForegroundColor DarkYellow
+                Write-Host "'$app' suggests installing '$([string]::join("' or '", $feature_suggestions))'." -ForegroundColor Cyan

If warnings aren’t yellow in practice, keeping DarkYellow is fine—please confirm.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 25d3500 and 6b2adae.

📒 Files selected for processing (2)
  • CHANGELOG.md (1 hunks)
  • lib/install.ps1 (2 hunks)

@z-Fng
Copy link
Author

z-Fng commented Sep 24, 2025

HUMORCE:
We could remove Notes character, when it are wrapped by separator. In some cases, it does not a 'note'.

coderabbitai:
358-364: Optional: drop the “Notes” label when wrapped by separators
Per review thread, removing the header reduces redundancy when the block is delimited above and below.

As mentioned above, is it better to remove 'Notes' characters when they are wrapped by separators? Like:

-Write-Output 'Notes'
Write-Output '-----'
Write-Output (wraptext (substitute $manifest.notes @{ '$dir' = $dir; '$original_dir' = $original_dir; '$persist_dir' = $persist_dir }))
Write-Output '-----'

Or make 'Notes' characters wrapped by separators too? Like this:

 Write-Output '-----'
 Write-Output 'Notes:'
 Write-Output (wraptext (substitute $manifest.notes @{ '$dir' = $dir; '$original_dir' = $original_dir; '$persist_dir' = $persist_dir }))
 Write-Output '-----'

@niheaven
Copy link
Member

niheaven commented Sep 24, 2025

Keep it as is now imo. A --- without title looks strange, right?

@z-Fng
Copy link
Author

z-Fng commented Sep 24, 2025

Keep it as is now imo. A --- without title looks strange, right?

Agree. This looks more natural imo.

  • Keep it as is now
image
  • Remove 'Notes' characters
image
  • Wrap 'Notes' by separators
image

@niheaven niheaven merged commit b592b38 into ScoopInstaller:develop Sep 24, 2025
3 checks passed
@z-Fng z-Fng deleted the opt-scoop-install branch September 24, 2025 09:01
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.

3 participants