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

Make zxcvbn infallible #78

Merged
merged 1 commit into from
May 11, 2024
Merged

Make zxcvbn infallible #78

merged 1 commit into from
May 11, 2024

Conversation

jrandolf
Copy link

There shouldn't be any reason for zxcvbn to error. An empty password should still be treated as valid input and we can use instant/performance.now to calculate durations accurately without overflow.

Copy link
Owner

@shssoichiro shssoichiro left a comment

Choose a reason for hiding this comment

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

Yeah, this makes a lot of sense. Thanks!

@jrandolf
Copy link
Author

Yeah, this makes a lot of sense. Thanks!

Could you re-approve the workflow? Also can you make a release after? I guess this will be a breaking change.

@jrandolf
Copy link
Author

jrandolf commented May 11, 2024

+ I think you should take a look at #77. It's better to write the score as an enumeration because it allows the compiler to pick the fastest representation for score (micro-optimization). It's a little incorrect because it should have #[non_exhaustive] to ensure the enum can be extendable without breaking changes. I can send a fixed version of that PR if you approve of that PR.

@shssoichiro
Copy link
Owner

I have mixed feelings on #77... From a performance standpoint, it's a micro-optimization that will be negligible compared to the other computations the library has to do, so I'm looking at it from a user experience standpoint. I'm probably just stuck in my old ways thinking that numbers are nice... Realistically, having an exhaustive enum is probably better. I can take another look at that one so the breaking changes can go out together.

@shssoichiro shssoichiro merged commit 3b81dce into shssoichiro:master May 11, 2024
5 checks passed
@shssoichiro
Copy link
Owner

It looks like that MR should be fixable with a simple merge commit, which I've pushed up. I'll review it shortly.

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.

2 participants