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 PasswordToken cloneable #184

Merged
merged 3 commits into from Apr 28, 2023
Merged

Make PasswordToken cloneable #184

merged 3 commits into from Apr 28, 2023

Conversation

roj1512
Copy link
Contributor

@roj1512 roj1512 commented Apr 27, 2023

No description provided.

@roj1512 roj1512 changed the title Make PasswordToken cloneable Make PasswordToken cloneable Apr 27, 2023
Copy link
Owner

@Lonami Lonami left a comment

Choose a reason for hiding this comment

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

Why is this change needed?

@roj1512
Copy link
Contributor Author

roj1512 commented Apr 27, 2023

So I can retry check_password() with the PasswordToken from SignInError::PasswordRequired when I get SignInError::PasswordInvalid.

@Lonami
Copy link
Owner

Lonami commented Apr 28, 2023

Good point. I would prefer if another solution was chosen, though, as making cloneable defeats the purpose of "making invalid states irrepresentable" (here, being able to use the token freely, even after successful sign-in).

What we should do is change the return type of check_password so that it includes the token every time. Then you can reuse the token if and only if the method failed and you're not logged in.

But I know that's a pain.

So please at least add:

// TODO this should not be Clone, but check_password Err doesn't include it back yet

above the change so we can remember to address it in the future. And then we can merge this temporary workaround.

(Unless you want to implement the better Err. That's the better solution.)

@roj1512
Copy link
Contributor Author

roj1512 commented Apr 28, 2023

check_password Err doesn't include it back yet

check_password error or SignInError::PasswordInvalid?

@Lonami
Copy link
Owner

Lonami commented Apr 28, 2023

The Error type used in check_password's Err variant should probably be a new enum specific to that method.

We probably want something as discussed in https://t.me/gramme_rs/17454. But that can come later. I'm happy with adding the TODO for now.

@roj1512
Copy link
Contributor Author

roj1512 commented Apr 28, 2023

Done!

@Lonami Lonami merged commit ec21116 into Lonami:master Apr 28, 2023
1 check passed
@roj1512 roj1512 deleted the patch-1 branch April 28, 2023 16:26
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.

None yet

3 participants