Skip to content

Conversation

EmersonRabelo
Copy link
Contributor

@profnandaa,
I have a possible improvement for the assertString.
I am reducing the number of necessary validations and also a more assertive approach, in my point of view (if this approach doesn't make sense, I would appreciate feedback on it, if possible).
The performance has also improved, it wasn't something incredible, but it was considerable.

image

Unit tests:
image

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.

I think the tests are failing because we currently do not throw an error for '' (an empty string).
Also; can you commit the unit tests as well?

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.95%. Comparing base (b958bd7) to head (f8e62de).
Report is 82 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2372      +/-   ##
==========================================
- Coverage   99.95%   99.95%   -0.01%     
==========================================
  Files         107      107              
  Lines        2454     2445       -9     
  Branches      619      617       -2     
==========================================
- Hits         2453     2444       -9     
  Partials        1        1              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@EmersonRabelo
Copy link
Contributor Author

EmersonRabelo commented Feb 9, 2024

Hi, @WikiRik!
I made a small change in the code and also committed the unit tests.
But it failed in one of the tests.

image

@WikiRik
Copy link
Member

WikiRik commented Feb 9, 2024

codecov/project will be fixed with #2341

WikiRik
WikiRik previously approved these changes Feb 9, 2024
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.

This looks good to me

rubiin
rubiin previously approved these changes Feb 9, 2024
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.

LGTM

@pano9000
Copy link
Contributor

pano9000 commented Feb 9, 2024

when you pass literally null, it gives

Uncaught TypeError: Cannot read properties of null (reading 'constructor')

because it is not caught by the first check.

Should we explicitly check for null there as well?

@EmersonRabelo
Copy link
Contributor Author

Hello, @pano9000!
Do you have any further comments on this or is everything okay?
If necessary, I will make the alterations!

Copy link
Contributor

@pano9000 pano9000 left a comment

Choose a reason for hiding this comment

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

should be ok now :-)
(however my approval is rather useless, as I am not a maintainer)

@EmersonRabelo EmersonRabelo dismissed stale reviews from rubiin and WikiRik via f8e62de June 1, 2024 14:35
@WikiRik WikiRik requested a review from rubiin March 27, 2025 18:33
@rubiin rubiin merged commit b610a88 into validatorjs:master Mar 28, 2025
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.

4 participants