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

Validator does not recognise £ as symbol #2148

Merged
merged 2 commits into from
Jan 22, 2023

Conversation

sandmule
Copy link
Contributor

Add £ to symbolRegex, for isStrongPassword.

Sorry for creating a double PR, but noticed that PR hadn't been touched in 5 days

@sandmule
Copy link
Contributor Author

@pano9000

@WikiRik
Copy link
Member

WikiRik commented Jan 16, 2023

With the speed of this repo, 5 days inactivity is nothing so in my opinion it is a bit early to duplicate the PR.

But in the case that the other PR (#2146) would not be updated; could you add a new test here? One that would pass when asking for a symbol in the password would be fine

@codecov
Copy link

codecov bot commented Jan 16, 2023

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (8259bba) compared to base (531dc7f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2148   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          104       104           
  Lines         2308      2308           
  Branches       578       578           
=========================================
  Hits          2308      2308           
Impacted Files Coverage Δ
src/lib/isStrongPassword.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sandmule
Copy link
Contributor Author

@WikiRik sorry about the impatience. It was raised internally today, so said I would take a look.

I've updated the test now.

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.

Changes look good to me.

That one failing CodeQL / Analyze (javascript) seems to be due to that service being/becoming deprecated:

https://github.blog/changelog/2022-04-27-code-scanning-deprecation-of-codeql-action-v1/

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.

Not exactly the test I had in mind, but I have some ideas to overhaul the test suite for this validator so we should be fine. If the other one does not get updated by the end of next week we can merge this for the upcoming release

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM. thanks for the fix 🎉

@profnandaa profnandaa merged commit ee81073 into validatorjs:master Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants