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

Namespace EmailValidator under the ValidEmail2 namespace. #79

Merged
merged 6 commits into from
Aug 2, 2017

Conversation

fredngo
Copy link
Contributor

@fredngo fredngo commented Jul 26, 2017

This is a breaking change. The invocation changes from:

validates :email, email: true

to

validates :email, 'valid_email_2/email': true

This solves #46

This is a breaking change. The invocation changes from:

  validates :email, email: true

to

  validates :email, 'valid_email_2/email': true

This solves micke#46
@fredngo
Copy link
Contributor Author

fredngo commented Jul 26, 2017

All support for Ruby 2.1 has ended, so it (along with 2.0) should probably be removed from the CI checks. Those are the only ones failing.

@micke
Copy link
Owner

micke commented Aug 2, 2017

Hi Fred. Thanks for working on this.

Wow, i had no idea you could namespace validations like this in rails.

I have two problems with this though;

First, as you already pointed out this is a breaking change. This will break peoples code which isn't that bad if we release it as v2.0, they'll have to come here on github and find out what changed or i could include a note in the after_install message in the gem (which i prefer not to).

It's kinda ugly imo, only because of the name of the gem 'valid_email_2/email': true is kind of a mouthful. This is only my fault because i had no imagination at all when i named the gem.

One solution that would make it backwards compatible and available namespaced at the same time is to include

class EmailValidator < ValidEmail2::EmailValidator
end

in email_validator.rb

This still means that we occupy the email validator and we are still available through the namespaced valid_email_2/email validator.
The best thing would've been if the validator was namespaced from the beginning though.

If people complain that the gem is occupying the email validator we could add a deprecation warning this way and eventually phase out the global validator, but i don't feel the need to do this right now.

What do you think?

@fredngo
Copy link
Contributor Author

fredngo commented Aug 2, 2017

Hi @lisinge !

I think your inheritance suggestion is a good idea. I'll go ahead and implement that today.

And then it'll be up to you whether to release as a 2.0 or less. I think people upgrading major versions should know that they contain breaking changes, so it's not a big deal IMO.

There is definitely a need for this due to the conflict with Devise... Led to strange behaviors in my own app for sure.

fredngo and others added 3 commits August 2, 2017 14:53
For now, the invocation can continue to be the same:

  validates :email, email: true
    
as well as:
    
  validates :email, 'valid_email_2/email': true
@micke micke merged commit e812c95 into micke:master Aug 2, 2017
@micke
Copy link
Owner

micke commented Aug 2, 2017

Great work! Thanks for your help!

@micke
Copy link
Owner

micke commented Aug 2, 2017

v2.0.0 is now released with this PR, i also added a deprecation warning with link to upgrade instructions in 3376119.

Thanks again!

@fredngo
Copy link
Contributor Author

fredngo commented Aug 2, 2017

Yay! Thanks for the release!

@micke micke mentioned this pull request Dec 28, 2017
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