Skip to content

Conversation

benjiyue
Copy link
Contributor

Addressed to #453

Added new constant max password fields to ValidationRules and added maximum password length annotation.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.0% when pulling 813d121 on benjiyue:master into 20ec84e on php-coder:master.

Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

First of all, thank you for the contribution!

I like this change and will merge it but I want to ask you about adding a few minor improvements:

  1. add a comment with explanation of why we're limiting password length
  2. squash your commits to a single, please
  3. could you specify your full name in the commit's author field (git config user.name "Full Name" && git commit --am --reset-author). It's not required but I believe that there is no reason to hide a full name when you're contributing to the open source and helping people :)
  4. it's a bit unusual that you post comments from one account but the patch from another. I want just warn you in case it was unintentionally and you forgot to switch to another account

public static final String NAME_NO_HYPHEN_REGEXP = "[ \\p{L}]([- \\p{L}]+[ \\p{L}])*";

public static final int PASSWORD_MIN_LENGTH = 4;
public static final int PASSWORD_MAX_LENGTH = 72;
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add a comment to this field and explain why we're limiting a password length. The comment should be the following:

// We limit max length because bcrypt has a maximum password length.
// See also: http://www.mscharhag.com/software-development/bcrypt-maximum-password-length

@php-coder
Copy link
Owner

P.S. Ideally I would like to have a test for that case. Are you interesting to providing it? (it's optional)

@php-coder
Copy link
Owner

P.P.S. If you're planning to contribute to this project again I would advise to create a separate branch for each PR next time. Having your work in master branch could lead to a problem with synchronization later on.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.0% when pulling ac4ece2 on benjiyue:master into 20ec84e on php-coder:master.

@benjiyue
Copy link
Contributor Author

benjiyue commented Mar 13, 2017

I added the comment and edited my full name. A long time ago I set my terminal to autologin to github using an older github account. I tried to change my ssh keys but it still uses my old account, so I just gave up and let it be. Can I work on the tests for this as another issue?

I tried squashing the commits, but I'm not sure if it worked, how can I check?

@php-coder
Copy link
Owner

Merged in 2ec30d9 commit.

@php-coder
Copy link
Owner

php-coder commented Mar 13, 2017

I added the comment and edited my full name.

I see, thanks!

Can I work on the tests for this as another issue?

Yes, sure. I've created separate issue for that: #545

I tried squashing the commits, but I'm not sure if it worked, how can I check?

I see your attempt. Instead of producing one commit that replaces 2 previous, you created yet another commit with a content of the 2 previous. Because of that fact I was able to merge it actually, but I hope that next time you will squash better :)

To check it, you can open your pull request on the GitHub and look at the 2nd tab "Commits". Right now it shows 6 commits but most of the time there should be only 1 commit.

I've added you to our "hall of fame": https://github.com/php-coder/mystamps/wiki/team-members
Also I've invited your to join this repository (this allows to assign tickets to you at least).

There is one step left after your changes were merged: you have to clean your master branch before updating it: git reset --hard 20ec84e141 Next time, please, do your changes in a separate branch.

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