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

Export analyzePassword Function #2468

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

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 profnandaa and pano9000 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.

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.

3 participants