supporting valid email (i.e. root@admin)

[jloveland]

supporting valid email (i.e. root@admin) according to HTML5 and RFC 822

fixing #934

[ilanbiala]

just "3 or more", not "more than 3 or more", no?

[jloveland]

fixed and squashed

[mleanos]

LGTM. With the exception of Ilan's comment on the wording of that test.

[ilanbiala]

@lirantal LGTM. When you merge, just add "Fixes #934" into the commit message to close and reference that commit.

[lirantal]

@ilanbiala sure, but this one gets applied for 0.4.2 or 0.5?

[codydaig]

LGTM. I'd say 0.4.2

[lirantal]

I agree.
@ilanbiala I'm not much of a fan of the 0.5 convention altogether because that derails us from the original branches that we had to begin with, and we left them for a reason. If we leave some PRs to be awaited to 0.5, which we don't know even when will it happen then 99% we're just going to re-do these PRs again due to conflicts and code-base that changed under the hood.

If we want to have items for a big/major release like 0.5 then I suggest we maybe have an 0.5 Roadmap issue where we can put these items on a taskboard, we can discuss them with regards to implementations etc, and once we're near that release we can go ahead and start working on a PR. And if some PRs get submitted which we think are obvious for an 0.5 then we should probably state that quite soon before contributors are investing time in contributing code which may never see it way into master, or will be very late in time, which is most often "too late".

[codydaig]

@lirantal I'll second that. If there are some major changes which would constitute a new minor version bump, then bump it then.

[ilanbiala]

@codydaig @lirantal you are breaking semver and it will mess up the versioning. This is 0.5.

[lirantal]

@ilanbiala how about we move the options for the validator package as seen here

/**
 * A Validation function for local strategy email
 */
var validateLocalStrategyEmail = function (email) {
  return ((this.provider !== 'local' && !this.updated) || validator.isEmail(email, {require_tld: false}));
};

to an external configuration variable, and revert the changes to make so that we have require_tld: true as originally in 0.4.1. This allow for users to better control the e-mail validation that they wish in their project without interfering with the core project code. Of course, they'd need to update their tests accordingly, but that's obvious.

WDYT?

[ilanbiala]

@lirantal I'm fine with not requiring a TLD, but it can only be published as part of 0.5.0. We don't need to wait for an accumulation of features for a release, we can just push 0.4.2 out sometime next week and then release 0.5.0 with whatever breaking changes we have.

[lirantal]

Sure, although I don't understand why you insist of pushing this feature in 0.5.0 even if it doesn't break anything. It's just a configuration option that defaults to 0.4.1 default so it doesn't break anything.

[ilanbiala]

@lirantal the configuration option seems unnecessary to me when we can just push it in a more intuitive and standards-conforming way as part of 0.5.0. Since it is a logical change and it changes functionality, it needs to be in a minor release according to semver.

[lirantal]

Cool.

[lirantal]

Ok

[codydaig]

@jloveland Just a heads up, the commit message guidelines have updated for any changes going into 0.5.0. Since this PR is just waiting the 0.4.2 release to get merged, if you want to edit the commit message it shall be ready. Thanks!
https://github.com/meanjs/mean/blob/master/CONTRIBUTING.md#commit-message-guidelines

[codydaig]

@jloveland Can you update the commit message and rebase? I think this is good to go.

[jloveland]

@codydaig I will sometime in the next couple days.

[jloveland]

@codydaig ready to go!

[codydaig]

@lirantal LGTM

[ilanbiala]

LGTM.

[roboflank]

LGTM

[lirantal]

Thanks guys, merging.