Skip to content

Conversation

@patrickfav
Copy link
Owner

refs #21

Copy link

@Indigo744 Indigo744 left a comment

Choose a reason for hiding this comment

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

Looks good, I have only a minor comment (and a bit of a rambling 😉 sorry couldn't help!)

README.md Outdated
Don't forget to use the same strategy when verifying:

```java
BCrypt.verifyer(LongPasswordStrategies.truncate()).verify(pw, hash)

Choose a reason for hiding this comment

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

You are missing a semicolon here 😉
Talking about it, I think the interface of the lib a bit surprising. Why BCrypt.with()... is for hashing, and BCrypt.verifyer()... is for verifying?
It would really make more sense, from an external point of view, if we could simply do:
BCrypt.with(LongPasswordStrategies.truncate()).verify(pw, hash);

I get that, internally, it is 2 different classes (too keep the code tidy I guess?) but it's not really obvious when using the lib.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I see your point. My understanding was that hashing and verifying are usually done in different places, so normally one would only do the one or other. I don't like to change the API lightly so I suggest we keep this fix with the current API and I'll create an improvement task where we can think about changing the API in a major version update.

Choose a reason for hiding this comment

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

Of course, let's keep focus on this PR.

Don't know if you seen my first sentence "You are missing a semicolon here"
It was kind of buried in my rambling 😅

Copy link
Owner Author

@patrickfav patrickfav Oct 20, 2019

Choose a reason for hiding this comment

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

Yes, just committed the update ;)

@coveralls
Copy link

coveralls commented Oct 20, 2019

Pull Request Test Coverage Report for Build 206

  • 8 of 8 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 96.058%

Totals Coverage Status
Change from base Build 204: 0.04%
Covered Lines: 463
Relevant Lines: 482

💛 - Coveralls

@patrickfav patrickfav merged commit 2b6befe into master Oct 20, 2019
@patrickfav patrickfav deleted the feature/21-verify-longpw branch October 20, 2019 13:25
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.

4 participants