[ENHANCEMENT] Add replaceEmoji() method #49
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new replaceEmoji() method to the LitEmoji library that allows replacing emoji with custom text while preserving non-emoji shortcode patterns. The implementation includes proper validation to only replace known emoji shortcodes.
- Added
replaceEmoji()method with callback-based validation using supported shortcodes - Refactored
removeEmoji()to use the newreplaceEmoji()method internally - Added comprehensive test coverage for the new functionality and edge cases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/LitEmoji.php | Implements the new replaceEmoji() method and refactors removeEmoji() to use it |
| tests/LitEmojiTest.php | Adds comprehensive test coverage for the new method and its edge cases |
| README.md | Documents the new replaceEmoji() method with usage examples |
| $supportedShortcodes = array_keys(self::getShortcodes()); | ||
|
|
||
| return preg_replace_callback('/:([\w_-]+):/', function($matches) use ($replacement, $supportedShortcodes) { | ||
| $shortcode = $matches[1]; | ||
|
|
||
| // Only replace if this is a known emoji shortcode | ||
| if (in_array($shortcode, $supportedShortcodes)) { |
There was a problem hiding this comment.
The array_keys(self::getShortcodes()) call on line 217 and the in_array() check on line 223 could be inefficient for large shortcode lists. Consider using array_key_exists($shortcode, self::getShortcodes()) instead to avoid creating the keys array and use O(1) lookup.
| $supportedShortcodes = array_keys(self::getShortcodes()); | |
| return preg_replace_callback('/:([\w_-]+):/', function($matches) use ($replacement, $supportedShortcodes) { | |
| $shortcode = $matches[1]; | |
| // Only replace if this is a known emoji shortcode | |
| if (in_array($shortcode, $supportedShortcodes)) { | |
| $shortcodes = self::getShortcodes(); | |
| return preg_replace_callback('/:([\w_-]+):/', function($matches) use ($replacement, $shortcodes) { | |
| $shortcode = $matches[1]; | |
| // Only replace if this is a known emoji shortcode | |
| if (array_key_exists($shortcode, $shortcodes)) { |
joshmcrae
left a comment
joshmcrae
left a comment
There was a problem hiding this comment.
Looks good, however for efficiency I would follow the recommendation Copilot made for checking whether a shortcode is valid before replacement.
Checking whether a key is in an array is faster than checking if an element is in an array - it's a constant time check vs linear time (taking longer as the list of shortcodes increases).
You could still put the shortcodes in a variable at line 217 instead of calling self::getShortcodes() for every encountered shortcode, otherwise the change it recommends is good.
Note: This change was made by Claude Code.
Summary
This builds off of the #31 Enhancement: Added replaceEmoji() method with proper validation based on reviewer feedback.
Key Improvements Made
Key Features
✅ Smart emoji detection: Only replaces actual emoji shortcodes
✅ Preserves custom shortcodes: Non-emoji :word: patterns remain unchanged
✅ Flexible replacement: Any string can be used as replacement text
✅ Backward compatible: Existing removeEmoji() functionality unchanged
✅ Well tested: Comprehensive test coverage with edge cases
✅ Well documented: Clear examples and usage patterns
The implementation addresses all the review feedback from PR #31 and provides a robust, validated emoji replacement feature that only affects actual emoji while preserving other shortcode-like patterns in the text.