fix: strip all punctuation characters #1482
Conversation
WalkthroughThe pull request introduces a new helper function, Assessment against linked issues
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kg-utils/lib/slugify.js (1)
28-28: Consider using a more maintainable approach for character definitionsFor improved readability and maintainability, consider extracting the regex pattern to a named constant. This would make future additions or modifications clearer.
module.exports = function (inputString = '', {ghostVersion = '4.0', type = 'mobiledoc'} = {}) { const version = semver.coerce(ghostVersion); + const PUNCTUATION_REGEX = /[\][!"#$%&'()*+,./:;<=>?@\\^_{|}~''""`¡¿–—•]/g; if (typeof inputString !== 'string' || (inputString || '').trim() === '') { return ''; } // ... return encodeURIComponent(inputString.trim() .toLowerCase() - .replace(/[\][!"#$%&'()*+,./:;<=>?@\\^_{|}~''""`¡¿–—•]/g, '') + .replace(PUNCTUATION_REGEX, '') .replace(/\s+/g, '-') .replace(/^-|-{2,}|-$/g, '') );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/kg-utils/lib/slugify.js(1 hunks)
🔇 Additional comments (2)
packages/kg-utils/lib/slugify.js (2)
28-28: Approve the expanded punctuation strippingThe change correctly extends the regex pattern to include additional punctuation and special characters (curly quotes, smart double quotes, backticks, inverted punctuation marks, dashes, and bullets) as specified in the PR objectives. This enhancement will create cleaner, more consistent slugs.
28-28:❓ Verification inconclusive
Consider adding unit tests for new punctuation characters
While the implementation is correct, it would be beneficial to add unit tests that verify the new punctuation characters are properly stripped from generated slugs.
🏁 Script executed:
#!/bin/bash # Check if there are existing tests for the slugify function that would cover this change echo "Searching for existing test files for slugify..." fd -t f --glob "*test*" | xargs grep -l "slugify" || echo "No tests found with 'slugify'"Length of output: 195
Action: Ensure Comprehensive Test Coverage for New Punctuation in Slugify
The implementation correctly strips the new punctuation characters as per the updated regex. I identified that there is an existing test file at
packages/kg-utils/test/slugify.test.js. Please review this file to confirm it includes specific test cases that validate the removal of each of the newly added punctuation characters. If coverage for these punctuation cases is missing or incomplete, kindly add the necessary tests.
|
@kevinansfield can you please have a look :D |
|
Hey @im-adithya 👋 Thanks for taking a look at this! Unfortunately we can't merge as-is because the change in slugify behaviour is a breaking change as mentioned here. We can work around that by putting the updated regex in a new version conditional like we currently have for |
|
Hey Kevin, sorry I missed that comment, I added the version conditional + tests, should be good now! (Also, can you please have a look at my other PRs as well? Thanks!) |
| ); | ||
| } | ||
|
|
||
| module.exports = function (inputString = '', {ghostVersion = '6.0', type = 'mobiledoc'} = {}) { |
There was a problem hiding this comment.
Just a heads up that I changed the default to 6.0
Fixes TryGhost/Ghost#22248
Description
Adds other punctuation characters as mentioned in the issue above to the regex.