Skip to content

Conversation

@THILAK6
Copy link

@THILAK6 THILAK6 commented Oct 1, 2024

Initially pointed out in #1966 to adapt isStrongPassword to return count of all type of characters in the password.
The responsibility of isStrongPassword should only be to check if the password is strong or not. So instead of modifying isStrongPassword, analyzePassword is exported which can be used to know the count of all type of characters in the password.

The issue is also attempted to solve here #2033

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@rubiin
Copy link
Member

rubiin commented Oct 1, 2024

I guess we should also include tests for it individually. Also could you fix the failed tests

@rubiin rubiin requested review from pano9000 and profnandaa and removed request for profnandaa October 1, 2024 16:48
@pano9000
Copy link
Contributor

pano9000 commented Oct 1, 2024

just my 2 cents here: but it feels like this would not really fit the scope of the project - you have validators and sanitizers in here, this analyzePassword function does not really fit into any category.

But this is not really my decision to make - so maybe to be discussed?

@THILAK6
Copy link
Author

THILAK6 commented Oct 2, 2024

I completely understand the concern regarding the scope of the project, as analyzePassword doesn’t fit neatly into the validator or sanitizer categories. Then we might need to fall back to the initial idea of extending the isStrongPassword function with an option to return analysis, but again I feel that's not the responsibility of isStrongPassword. I'm happy to take it in either direction based on what fits best with the project’s goals.

@binarykitchen
Copy link

Hello all, any updates on this? I'm totally keen on this feature for my website, please. Thanks :)

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.

4 participants