-
Notifications
You must be signed in to change notification settings - Fork 1
fix(markdown): strip Slack emoji skin tone suffixes #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hi @daabr 👋 Please let me know if you’d prefer normalization instead of stripping. |
daabr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Sarthakkeche!
Looks like neither GitHub nor Bitbucket Cloud support markdown codes for skins tones like Slack, so the recommended practice for conversion is to use the relevant Unicode characters instead of markdown codes.
But that would be a lot more work, so your approach is fine for now.
Please address my review comments. Also, this PR shouldn't delete any existing comment lines in the code.
| } | ||
|
|
||
| // stripSlackEmojiSkinTones removes Slack-style emoji skin tone suffixes. | ||
| func stripSlackEmojiSkinTones(text string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't redeclare a function with the same name multiple times in the same Go package, even if i's in separate files. Please remove this function in this file.
| text = regexp.MustCompile(`(?m)^(#+)\s+(.+)`).ReplaceAllString(text, "*${1} ${2}*") | ||
|
|
||
| // Fix header lines with explicit boldface: "# **...**" --> "*# ...*". | ||
| return regexp.MustCompile(`(?m)^\*(#+) \*(.+?)\*\*`).ReplaceAllString(text, "*${1} ${2}*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this line?
| // Examples: | ||
| // :thumbsup_tone2: | ||
| // :thumbsup::skin-tone-3: | ||
| func stripSlackEmojiSkinTones(text string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this function is reused by Bitbucket and GitHub, please move it from this file to a new file called emoji.go.
Also, don't forget to add unit tests in a new file called emoji_test.go.
| // :thumbsup_tone2: | ||
| // :thumbsup::skin-tone-3: | ||
| func stripSlackEmojiSkinTones(text string) string { | ||
| re := regexp.MustCompile(`(_tone[1-6])|(::skin-tone-[1-6])`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Out of curiosity, where is
_tone[1-6]coming from? Got any example for this? - In case (1) is not a mistake, a simpler regular expression would be:
(_tone|::skin-tone-)[1-6]
This PR strips Slack-style emoji skin tone suffixes during markdown
conversion for GitHub and Bitbucket to ensure consistent rendering.
Closes #23