Skip to content

fix: address PR #2 review feedback#4

Merged
chiply merged 3 commits intomainfrom
fix/pr2-review-feedback
Feb 19, 2026
Merged

fix: address PR #2 review feedback#4
chiply merged 3 commits intomainfrom
fix/pr2-review-feedback

Conversation

@chiply
Copy link
Owner

@chiply chiply commented Feb 19, 2026

Summary

  • Assert stringp return from brushup--eval-style on error (the message call in the condition-case handler returns a string, not nil)
  • Replace || true on eask install-deps with exit-code-2-specific handling so real dependency resolution failures fail the job clearly (exit 2 = no deps to install, which is expected)
  • Add inline comments explaining the Nix cache steps are needed because jcs090218/setup-emacs uses Nix to install Emacs on Linux

Review items addressed

Items already resolved in prior commits: lexical binding for eval-style and brushup-test--marker (both use defvar).

Test plan

  • CI passes on all matrix jobs (exit code handling works cross-platform)
  • handles-error test still passes with stringp assertion

Generated with Claude Code

- Assert stringp return from brushup--eval-style on error (message returns string)
- Replace `|| true` with exit-code-2-specific handling so real dep failures aren't masked
- Add comments explaining Nix cache is needed by setup-emacs on Linux

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 19, 2026 23:06
GitHub Actions uses set -eo pipefail by default, so the semicolon
pattern exits immediately on non-zero before rc=$? runs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses review feedback from PR #2, focusing on three specific improvements: better test coverage for error handling, more robust CI dependency installation, and explanatory comments for Nix cache usage.

Changes:

  • Enhanced error handling test for brushup--eval-style to verify it returns a string message instead of just not signaling
  • Replaced || true with proper exit code handling for eask install-deps to allow exit code 2 (no dependencies) while failing on actual errors
  • Added inline comments explaining Nix cache steps are needed because jcs090218/setup-emacs uses Nix on Linux

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
test/brushup-test.el Added stringp assertion to verify error handler returns a message string
.github/workflows/ci.yml Improved CI robustness with proper exit code handling and added explanatory comments for Nix cache

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -82,7 +85,9 @@ jobs:
key: eask-lint-${{ hashFiles('Eask') }}

- name: Install dependencies
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

The shell: bash directive is missing in the lint job's "Install dependencies" step, but it's present in the test job's equivalent step (line 52). This inconsistency could cause portability issues since the bash-specific syntax ([ ... ] || [ ... ] || exit ...) is used in the run script. While ubuntu-latest defaults to bash, it's better to be explicit for consistency and clarity.

Suggested change
- name: Install dependencies
- name: Install dependencies
shell: bash

Copilot uses AI. Check for mistakes.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@chiply chiply merged commit c8755ae into main Feb 19, 2026
7 checks passed
@chiply chiply mentioned this pull request Feb 19, 2026
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