Skip to content

isEmojy added. #1890

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

isEmojy added. #1890

wants to merge 3 commits into from

Conversation

erfiboy
Copy link

@erfiboy erfiboy commented Dec 23, 2021

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f055c11) to head (ea013c5).
Report is 216 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1890   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          102       102           
  Lines         2072      2072           
  Branches       472       472           
=========================================
  Hits          2072      2072           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Don't forget to add tests

@@ -111,6 +111,7 @@ Validator | Description
**isEAN(str)** | check if the string is an EAN (European Article Number).
**isEmail(str [, options])** | check if the string is an email.<br/><br/>`options` is an object which defaults to `{ allow_display_name: false, require_display_name: false, allow_utf8_local_part: true, require_tld: true, allow_ip_domain: false, domain_specific_validation: false, blacklisted_chars: '', host_blacklist: [] }`. If `allow_display_name` is set to true, the validator will also match `Display Name <email-address>`. If `require_display_name` is set to true, the validator will reject strings without the format `Display Name <email-address>`. If `allow_utf8_local_part` is set to false, the validator will not allow any non-English UTF8 character in email address' local part. If `require_tld` is set to false, e-mail addresses without having TLD in their domain will also be matched. If `ignore_max_length` is set to true, the validator will not check for the standard max length of an email. If `allow_ip_domain` is set to true, the validator will allow IP addresses in the host part. If `domain_specific_validation` is true, some additional validation will be enabled, e.g. disallowing certain syntactically valid email addresses that are rejected by GMail. If `blacklisted_chars` receives a string, then the validator will reject emails that include any of the characters in the string, in the name part. If `host_blacklist` is set to an array of strings and the part of the email after the `@` symbol matches one of the strings defined in it, the validation fails.
**isEmpty(str [, options])** | check if the string has a length of zero.<br/><br/>`options` is an object which defaults to `{ ignore_whitespace:false }`.
**isEmojy(str)** | check if the string contains any emojis.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**isEmojy(str)** | check if the string contains any emojis.
**isEmoji(str)** | check if the string contains any emojis.

@@ -0,0 +1,7 @@
import assertString from './util/assertString';

export default function isEmojy(str) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export default function isEmojy(str) {
export default function isEmoji(str) {

Copy link
Member

@rubiin rubiin left a comment

Choose a reason for hiding this comment

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

Add tests please. That would ensure fast approval

Copy link
Member

@rubiin rubiin left a comment

Choose a reason for hiding this comment

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

Also the file name and validator name, plese rename to isEmoji

@profnandaa profnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Apr 22, 2022
@profnandaa
Copy link
Member

Thanks for this, please make the changes suggested and we should land this in.

@WikiRik WikiRik mentioned this pull request May 17, 2022
3 tasks

export default function isEmojy(str) {
assertString(str);
const regexExp = /(\u00a9|\u00ae|[\u2000-\u3300]|\ud83c[\ud000-\udfff]|\ud83d[\ud000-\udfff]|\ud83e[\ud000-\udfff])/gi;
Copy link

Choose a reason for hiding this comment

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

It's broken expression.
image
It matches Korean character. #1968

@pano9000
Copy link
Contributor

pano9000 commented Dec 1, 2022

one tiny additional issue I see here:

What is the intended goal here?

  • a) check if a string contains any emojis
    OR
  • b) check if the string IS an emoji

Both of which could be valid use cases, I guess, but in any case expectation and result should match:

Currently the validator is called "isEmoji" - and to me this suggests that it tests case b)
Howecer it actually tests case a) instead:

E.g. isEmojy("abc😂def") will return true

EDIT+
Ok, somehow I missed, that #1968 exists
That PR is adressing the issue above, and went for checking for case b) :-)

@rubiin rubiin added the duplicate Duplicate issue label Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate Duplicate issue 🧹 needs-update For PRs that need to be updated before landing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants