Skip to content
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

fix: yat grapheme validation #3909

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Feb 24, 2023

Description

Fixes y.at validation, currently failing for some emojis with the current logic.
The tl;dr on that is that /^\p{Extended_Pictographic}{1,5}$/u doesn't test/match what we would naively think it does.

  • '🦊❤️😎❤️'.match(/\p{Extended_Pictographic}/gu) // ["🦊","❤","😎","❤"] (notice the removed caret and dollarsign as well as the 1,5 multiplicator)
  • '🦊❤️😎❤️'.match(/\p{Extended_Pictographic}+/gu) // [ "🦊❤", "😎❤" ] (notice the + multiplicator, and how this now creates two groups of two "emojis")
  • '🦊❤️😎❤️'.match(/^\p{Extended_Pictographic}{1,5}^/gu) // null

Fixed by using the grapheme-splitter as recommended by Gabriel from y.at, which ensures we split characters by graphemes e.g "user-visible characters", as opposed to actual characters, which emojis might be made of many.

Notice

  • Have you followed the guidelines in our Contributing guide?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

N/A

Risk

None, worst case scenario this is still broken

Testing

  • Ensure valid y.ats now pass validation
  • Invalid y.ats (not registered, or with some non-emojis characters somewhere in the string) are still failing validation

Engineering

☝🏽

Operations

☝🏽

Screenshots (if applicable)

image

image

image

image

image

image

Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@gomesalexandre gomesalexandre marked this pull request as ready for review February 24, 2023 04:55
@gomesalexandre gomesalexandre requested a review from a team as a code owner February 24, 2023 04:55
Copy link
Collaborator

@0xdef1cafe 0xdef1cafe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very naice

@0xdef1cafe 0xdef1cafe enabled auto-merge (squash) February 24, 2023 07:58
@0xdef1cafe 0xdef1cafe merged commit f9ea73f into develop Feb 24, 2023
@0xdef1cafe 0xdef1cafe deleted the fix_yat_grapheme_validation branch February 24, 2023 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants