Skip to content

[#411] Ended every console, log and exception message with a period and quoted interpolated values. - #415

Merged
AlexSkrypnyk merged 5 commits into
mainfrom
feature/411-message-style
Oct 1, 2026
Merged

AlexSkrypnyk merged 5 commits into
mainfrom
feature/411-message-style

Conversation

@AlexSkrypnyk

Copy link
Copy Markdown
Member

Closes #411

Summary

Console lines (writeln()), log entries ($this->logger->*()) and exception messages that read as a sentence now end with a period, and every interpolated value in them is wrapped in double quotes, with each item of a list quoted on its own (Allowed modes are: "force-push", "branch".). That's the form Symfony Console's own messages already use (The "--foo" option does not exist.). The convention covers ArtifactCommand, ArtifactGitRepository, FilesystemTrait, LoggerTrait and BranchNotFoundException, and CLAUDE.md states it under Code Standards. FilesystemTrait::fsRealpath() also puts the original symlink path into its Could not resolve symlink for path: error.

The messages followed no single rule. A successful run printed Pushed branch "x" with commit message "y" without a period while Artifact packaged successfully. had one, and a --branch value that failed validation printed Incorrect value "x" specified for git remote branch with the value quoted while an invalid remote printed Invalid remote URL provided: git://user/repo with the value bare. Separately, fsRealpath() assigned the return value of readlink() straight back to $path, so when readlink() returned FALSE the Could not resolve symlink for path: exception was built from the overwritten $path and printed nothing after the colon, and the !$path check also rejected a symlink whose target is the string 0.

After merge, a successful run prints Pushed branch "artifact" with commit message "Deployment commit"., and a CI grep for an old string still matches wherever the old string is a prefix of the new one, which is every changed message except those whose value gained quotes, such as Invalid remote URL provided: git://user/repo. Label-value lines (Commit: <hash>, Added changes: <git output>), the info and report table rows, the ----- separators with the raw .gitignore dump, and the Processing failed with an error: heading are data and keep their form. Messages that end by appending another exception's message, such as the chained Unable to determine source branch. Artifact packaging failed. exception, are unchanged because the inner message supplies its own period.

Before / After

Console lines, log entries and exception messages, using real strings from the source:

┌─ BEFORE ─────────────────────────────────────────────────────────────────────┐
│ console    Pushed branch "artifact" with commit message "Deployment commit"  │
│ exception  Invalid remote URL provided: git://user/repo                      │
│ exception  Invalid mode provided. Allowed modes are: force-push, branch      │
│ log        Removing ignored file /path/src/ignored file                      │
│ log        Cleaning up                                                       │
└──────────────────────────────────────────────────────────────────────────────┘
                                       │
                                       ▼
┌─ AFTER ──────────────────────────────────────────────────────────────────────┐
│ console    Pushed branch "artifact" with commit message "Deployment commit". │
│ exception  Invalid remote URL provided: "git://user/repo".                   │
│ exception  Invalid mode provided. Allowed modes are: "force-push", "branch". │
│ log        Removing ignored file "/path/src/ignored file".                   │
│ log        Cleaning up.                                                      │
└──────────────────────────────────────────────────────────────────────────────┘

fsRealpath() on a symlink, by what readlink() returns:

┌───────────────────┬──────────────────────────────┬──────────────────────────────────┐
│ readlink() result │ BEFORE                       │ AFTER                            │
├───────────────────┼──────────────────────────────┼──────────────────────────────────┤
│ FALSE             │ throws, path printed empty   │ throws, original path printed    │
│ "0"               │ throws, treated as a failure │ continues with "0" as the target │
│ "/real/target"    │ continues with the target    │ continues with the target        │
└───────────────────┴──────────────────────────────┴──────────────────────────────────┘

Changes

Messages

  • src/Commands/ArtifactCommand.php: console lines, log entries and exceptions end with a period, Unable to load contents of quotes the .gitignore path, and setMode() passes the 2 mode constants straight to sprintf() instead of imploding an array.
  • src/Git/ArtifactGitRepository.php: the Disabling local exclude, Restoring local exclude, Removing ignored file, Removing other file, Removing sub-repository, Copying custom .gitignore file and Restoring custom .gitignore file debug lines, the Invalid remote URL provided and Invalid argument exceptions, and both Unable to determine a detachment source throws follow the convention. The first BranchNotFoundException throw in getOriginalBranch() is collapsed onto 1 line.
  • src/Traits/FilesystemTrait.php: fsAssertPathsExist() quotes each path in the list it reports, so a, b becomes "a", "b".
  • src/Traits/LoggerTrait.php and src/Exception/BranchNotFoundException.php: Debug messages enabled. and the default message Unable to determine source branch. end with a period.

Symlink resolution

  • fsRealpath() holds the result of readlink() in $target, throws with the original $path when $target === FALSE, and only then assigns $path = $target. The strict comparison also stops a symlink whose target is the string 0 from being treated as a failure. The throw sits inside @codeCoverageIgnore, so no test drives it.

Tests

  • Every assertion on a changed message checks the full new text, trailing period included. The negative assertStringNotContainsString() checks keep the shorter text on purpose.
  • GeneralTest::testDebug() gains assertions for Checking requirements., All requirements were met. and Cleaning up..
  • ForcePushModeTest::testGitignoreCustomRemoveCommittedFiles() runs at -vvv and asserts the custom .gitignore copy and restore lines plus Removing ignored file "..." for 2 files.
  • FilesystemTraitTest::testFsAssertPathsExistWithNonExistingPathStrict() is driven by a data provider covering 1 missing path, 2 missing paths and a path containing spaces.
  • MissingBranchTest asserts the full chained message Unable to determine source branch. Artifact packaging failed. Unable to determine a detachment source., and ArtifactCommandTest asserts the full Unable to determine the remote default branch; skipping stale cleanup for safety. text.

Docs

  • CLAUDE.md: a bullet under Code Standards states the convention.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 16 billable files and costs up to $4.00.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 8 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available. Your 84 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: 483a295b-877d-4b13-956f-3e385408e61a

📥 Commits

Reviewing files that changed from the base of the PR and between d04d240 and 6c35616.

📒 Files selected for processing (16)
  • CLAUDE.md
  • src/Commands/ArtifactCommand.php
  • src/Exception/BranchNotFoundException.php
  • src/Git/ArtifactGitRepository.php
  • src/Traits/FilesystemTrait.php
  • src/Traits/LoggerTrait.php
  • tests/Functional/BranchModeTest.php
  • tests/Functional/CleanupStaleBranchesTest.php
  • tests/Functional/ForcePushModeTest.php
  • tests/Functional/FunctionalTestCase.php
  • tests/Functional/GeneralTest.php
  • tests/Functional/MissingBranchTest.php
  • tests/Unit/Commands/ArtifactCommandTest.php
  • tests/Unit/Exception/BranchNotFoundExceptionTest.php
  • tests/Unit/Git/ArtifactGitRepositoryTest.php
  • tests/Unit/Traits/FilesystemTraitTest.php
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.71%. Comparing base (d04d240) to head (6c35616).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/Git/ArtifactGitRepository.php 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
+ Coverage   97.16%   97.71%   +0.54%     
==========================================
  Files           6        6              
  Lines         530      526       -4     
==========================================
- Hits          515      514       -1     
+ Misses         15       12       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@AlexSkrypnyk AlexSkrypnyk added the Needs review Pull request needs a review from assigned developers label Oct 1, 2026
@AlexSkrypnyk
AlexSkrypnyk merged commit 9267b68 into main Oct 1, 2026
12 checks passed
@AlexSkrypnyk
AlexSkrypnyk deleted the feature/411-message-style branch October 1, 2026 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs review Pull request needs a review from assigned developers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Choose 1 punctuation and quoting style for console and exception messages

2 participants