Add ability to customize separator across Onym strategies via options and constructor#1
Conversation
|
Thanks for this addition I can see the value. I just wanted to acknowledge the request I'll try and look as this and get this merged later today. If not it will be Wednesday as im travelling Monday / Tuesday. I just wanted to let you know and set some expectation. Your contribution is appreciated Have a great day |
|
Would it be possible to merge this please 🙏 |
- illuminate/support now allows ^13.0 - dev toolchain: testbench ^10|^11, phpunit ^11|^12 - CI matrix exercises Laravel 12/13 on PHP 8.3/8.4 - suite passes on Laravel 13.15 (34 tests) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a configurable ChangesDefault Separator Configuration
Multi-Version Testing Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Onym.php (1)
96-96:⚠️ Potential issue | 🟠 MajorPass
$useOptionsinto theslugstrategy call
make()calls$this->slug($defaultFilename, $extension)without$useOptions, unlike the other strategies, so custom options (e.g.,separator) are ignored whenstrategy='slug'.🔧 Proposed fix
- 'slug' => $this->slug($defaultFilename, $extension), + 'slug' => $this->slug($defaultFilename, $extension, $useOptions),Add a test that calls
make()withstrategy='slug'and a customseparator, asserting the produced slug uses that separator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Onym.php` at line 96, The make() call is invoking $this->slug($defaultFilename, $extension) without passing $useOptions so custom slug options (e.g., separator) are ignored; update the call in Onym::make() to pass $useOptions into the slug strategy ($this->slug($defaultFilename, $extension, $useOptions)) and ensure Onym::slug accepts and uses that parameter; add a unit test that calls make() with strategy='slug' and a custom 'separator' option and asserts the returned slug uses the provided separator.
🧹 Nitpick comments (2)
.github/workflows/main.yml (1)
23-30: ⚡ Quick winConsider security hardening recommendations from static analysis.
The static analysis tool flagged two security concerns:
- Unpinned action references (lines 24, 27): Actions reference tags (
@v4,@v2) rather than commit SHAs, which exposes the workflow to supply-chain attacks if tags are moved.- Credential persistence (line 24): The checkout action doesn't set
persist-credentials: false, which means GitHub tokens persist in the workspace and could leak through artifacts.These are security best practices recommended by OpenSSF. However, many projects accept these risks for maintainability since tags are more readable than commit SHAs.
🔒 Proposed hardening changes
- name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false - name: Set up PHP - uses: shivammathur/setup-php@v2 + uses: shivammathur/setup-php@c541c155eee45413f5b09a52d31318011562d79a # v2.31.1 with:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/main.yml around lines 23 - 30, Update the GitHub Actions workflow to address the static-analysis security flags: replace the loose action tags actions/checkout@v4 and shivammathur/setup-php@v2 with pinned commit SHAs (use the full commit SHA for each action instead of the tag) and add persist-credentials: false to the checkout step (the actions/checkout usage) so the GITHUB_TOKEN is not left in the workspace; keep the existing php-version and extensions inputs when updating the pin.Source: Linters/SAST tools
tests/OnymTest.php (1)
214-291: 💤 Low valueConsider consistent test naming convention.
The new tests use
test_*prefix while existing tests in the file useit_*prefix. For consistency within the test suite, consider renaming to match the established pattern (e.g.,it_uses_default_separator_for_timestamp,it_overrides_separator_for_timestamp).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/OnymTest.php` around lines 214 - 291, The test methods use a different naming convention (test_*) than the rest of the file (it_*); rename the new methods (test_timestamp_uses_default_separator, test_timestamp_overrides_separator_in_options, test_date_uses_default_separator, test_date_overrides_separator_in_options, test_numbered_uses_default_separator, test_numbered_overrides_separator_in_options, test_slug_uses_default_separator, test_slug_overrides_separator_in_options) to follow the existing it_* pattern (e.g., it_uses_default_separator_for_timestamp, it_overrides_separator_for_timestamp, etc.), keeping the same #[Test] attributes and test bodies so the assertions and referenced symbols (Onym->timestamp, Onym->date, Onym->numbered, Onym->slug) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@composer.json`:
- Line 19: The composer.json PHP platform constraint currently excludes PHP 8.1;
update the "php" constraint entry (the "php" key in composer.json) to include
^8.1 (e.g., "php": "^8.0|^8.1|^8.2|^8.3|^8.4") if 8.1 is supported, or
alternatively add a short note in project docs (README or composer.json
comments) explaining the intentional exclusion of 8.1 and the reason; modify the
"php" value under the composer.json file or add the documentation accordingly so
the constraint matches the project compatibility decision.
In `@README.md`:
- Around line 274-276: The README example calling $filename =
$onym->timestamp('invoice', 'pdf'); uses the default timestamp format
'Y-m-d_H-i-s' but shows a date-only result; update the example so it matches
reality by either calling $onym->timestamp('invoice', 'pdf', ['format' =>
'Y-m-d']) to produce "2024-03-15|invoice.pdf" or change the expected result to
include the full default timestamp (e.g., "2024-03-15_14-30-00|invoice.pdf");
reference the timestamp() call and the default format 'Y-m-d_H-i-s' when making
the change.
---
Outside diff comments:
In `@src/Onym.php`:
- Line 96: The make() call is invoking $this->slug($defaultFilename, $extension)
without passing $useOptions so custom slug options (e.g., separator) are
ignored; update the call in Onym::make() to pass $useOptions into the slug
strategy ($this->slug($defaultFilename, $extension, $useOptions)) and ensure
Onym::slug accepts and uses that parameter; add a unit test that calls make()
with strategy='slug' and a custom 'separator' option and asserts the returned
slug uses the provided separator.
---
Nitpick comments:
In @.github/workflows/main.yml:
- Around line 23-30: Update the GitHub Actions workflow to address the
static-analysis security flags: replace the loose action tags
actions/checkout@v4 and shivammathur/setup-php@v2 with pinned commit SHAs (use
the full commit SHA for each action instead of the tag) and add
persist-credentials: false to the checkout step (the actions/checkout usage) so
the GITHUB_TOKEN is not left in the workspace; keep the existing php-version and
extensions inputs when updating the pin.
In `@tests/OnymTest.php`:
- Around line 214-291: The test methods use a different naming convention
(test_*) than the rest of the file (it_*); rename the new methods
(test_timestamp_uses_default_separator,
test_timestamp_overrides_separator_in_options, test_date_uses_default_separator,
test_date_overrides_separator_in_options, test_numbered_uses_default_separator,
test_numbered_overrides_separator_in_options, test_slug_uses_default_separator,
test_slug_overrides_separator_in_options) to follow the existing it_* pattern
(e.g., it_uses_default_separator_for_timestamp,
it_overrides_separator_for_timestamp, etc.), keeping the same #[Test] attributes
and test bodies so the assertions and referenced symbols (Onym->timestamp,
Onym->date, Onym->numbered, Onym->slug) remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c43112d-a1d5-4b25-b924-542b3ec21b30
📒 Files selected for processing (7)
.github/workflows/main.ymlREADME.mdcomposer.jsonconfig/config.phpsrc/Onym.phptests/OnymTest.phptests/TestCase.php
| @@ -17,11 +17,11 @@ | |||
| ], | |||
| "require": { | |||
| "php": "^8.0|^8.2|^8.3|^8.4", | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for PHP 8.1 related comments or documentation
# Check README and docs for PHP version mentions
fd -e md -e txt | xargs rg -i 'php.*8\.1' -C2
# Check for any PHP version related issues or comments in code
rg -i 'php.*8\.1|8\.1.*incompatible|8\.1.*issue' -C2Repository: Blaspsoft/onym
Length of output: 40
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show composer.json php constraint with line numbers around it
echo "== composer.json =="
if [ -f composer.json ]; then
nl -ba composer.json | sed -n '1,120p'
else
echo "composer.json not found at repo root"
fi
# 2) Search for explicit PHP 8.1 mentions/incompat notes across the repo (excluding vendor)
echo "== rg for PHP 8.1 related mentions =="
rg -n --hidden --glob '!vendor/**' --glob '!node_modules/**' --glob '!storage/**' \
'(php[^0-9]{0,10}8\.1|8\.1[^0-9]{0,10}(incompatible|not supported|not supported|unsupported)|8\.1.*(incompatible|issue|break|fails|problem|support))' \
-S || true
# 3) Broad search for "8.1" occurrences to see any context
echo "== rg for literal 8.1 (broad) =="
rg -n --hidden --glob '!vendor/**' --glob '!node_modules/**' \
'8\.1' -S || trueRepository: Blaspsoft/onym
Length of output: 139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ls =="
ls -la | head
echo "== composer.json =="
if [ -f composer.json ]; then
# Print first 120 lines with line numbers via awk
awk '{printf "%5d\t%s\n", NR, $0}' composer.json | sed -n '1,120p'
else
echo "composer.json not found at repo root"
fi
echo "== rg for PHP 8.1 related mentions =="
rg -n --hidden --glob '!vendor/**' --glob '!node_modules/**' \
'(php[^0-9]{0,10}8\.1|8\.1[^0-9]{0,10}(incompatible|not supported|unsupported|not supported|problem|fails|issue)|8\.1.*(incompatible|issue|break|fails|problem|support))' \
-S || true
echo "== rg for literal 8.1 occurrences (broad) =="
rg -n --hidden --glob '!vendor/**' --glob '!node_modules/**' \
'8\.1' -S || trueRepository: Blaspsoft/onym
Length of output: 2234
Add ^8.1 back to the PHP constraint (or document why it’s excluded).
composer.json (line 19) sets "php": "^8.0|^8.2|^8.3|^8.4", which excludes PHP 8.1 and prevents installs on that runtime. There are no repo notes/comments indicating PHP 8.1 is incompatible, so please confirm whether to include ^8.1 or document the deliberate exclusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@composer.json` at line 19, The composer.json PHP platform constraint
currently excludes PHP 8.1; update the "php" constraint entry (the "php" key in
composer.json) to include ^8.1 (e.g., "php": "^8.0|^8.1|^8.2|^8.3|^8.4") if 8.1
is supported, or alternatively add a short note in project docs (README or
composer.json comments) explaining the intentional exclusion of 8.1 and the
reason; modify the "php" value under the composer.json file or add the
documentation accordingly so the constraint matches the project compatibility
decision.
| $filename = $onym->timestamp('invoice', 'pdf'); | ||
|
|
||
| // Result: "2024-03-15|invoice.pdf" |
There was a problem hiding this comment.
Correct the example output to match the default timestamp format.
The example calls timestamp() without options, so it uses the default format 'Y-m-d_H-i-s' which includes time. The actual result would be "2024-03-15_14-30-00|invoice.pdf", not "2024-03-15|invoice.pdf".
To fix, either:
- Pass
['format' => 'Y-m-d']to produce date-only output, or - Update the expected result to show the full timestamp with time
📝 Proposed fix (option 1: use date-only format)
// Use the timestamp strategy with the custom separator
-$filename = $onym->timestamp('invoice', 'pdf');
+$filename = $onym->timestamp('invoice', 'pdf', ['format' => 'Y-m-d']);
// Result: "2024-03-15|invoice.pdf"Or option 2: update the expected result comment:
// Use the timestamp strategy with the custom separator
$filename = $onym->timestamp('invoice', 'pdf');
-// Result: "2024-03-15|invoice.pdf"
+// Result: "2024-03-15_14-30-00|invoice.pdf"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $filename = $onym->timestamp('invoice', 'pdf'); | |
| // Result: "2024-03-15|invoice.pdf" | |
| $filename = $onym->timestamp('invoice', 'pdf', ['format' => 'Y-m-d']); | |
| // Result: "2024-03-15|invoice.pdf" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 274 - 276, The README example calling $filename =
$onym->timestamp('invoice', 'pdf'); uses the default timestamp format
'Y-m-d_H-i-s' but shows a date-only result; update the example so it matches
reality by either calling $onym->timestamp('invoice', 'pdf', ['format' =>
'Y-m-d']) to produce "2024-03-15|invoice.pdf" or change the expected result to
include the full default timestamp (e.g., "2024-03-15_14-30-00|invoice.pdf");
reference the timestamp() call and the default format 'Y-m-d_H-i-s' when making
the change.
2 participants
Description
This pull request enhances the Onym package by making the separator used across different strategies fully customizable.
Previously, the
timestamp(),date(), andnumbered()methods hard-coded the separator (e.g., underscore_) into their generated filenames. There was no way to override it dynamically through options.This PR introduces:
separatoroption in all relevant methods (timestamp,date,numbered, andslug).defaultSeparatorif noseparatoris provided in options.optionsanddefaultSeparatorthrough the constructor, making the class easier to customize and test.Example usage: