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

feature: Change to a Rails 4 ActiveModel validator. #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schwern
Copy link

@schwern schwern commented Feb 17, 2020

Rails 4 introduced ActiveModel, ActiveModel::Validations and ActiveModel::EachValidator. Instead of injecting ourselves into ActiveRecord::Base, we will be automatically picked up. This fixes #10.

We can now also be used on ActiveModel as well as ActiveRecord. This simplifies testing.

Also

  • rspec-rails was dropped becasue it seems to want all of Rails loaded.
    We only needed one convenience method, errors_on. It has been copied into our test model.
  • Bumped our runtime dependency to activemodel >= 4.0. Dropped the others.
  • Dropped the development dependency on all of Rails. We only need ActiveModel.

Rails 4 introduced ActiveModel, ActiveModel::Validations and
ActiveModel::EachValidator.

Instead of injecting ourselves into ActiveRecord::Base, we will be
automatically picked up. This fixes KimNorgaard#10.

We can now also be used on ActiveModel as well as ActiveRecord. This
simplifies testing.

Also
* rspec-rails was dropped becasue it seems to want all of Rails loaded.
  We only needed one convenience method, errors_on. It has been copied into our test model.
* Bumped our runtime dependency to activemodel >= 4.0. Dropped the others.
* Dropped the development dependency on all of Rails. We only need ActiveModel.
@KimNorgaard
Copy link
Owner

I am in a bit of a pickle here since I havn't touched rails since v3. How does one go about supporting backwards compatibility for a gem such as this?

@schwern
Copy link
Author

schwern commented Dec 7, 2022

@KimNorgaard Since most of the code remains the same, we could write a module with all the common code, and then maintain Rails v3 and Rails v4+ wrapper files and load one or the other as appropriate for the Rails version.

However, considering that the last release of Rails v3 was 10 years ago, I would say it's much simpler to bump to version 2 to indicate a breaking change. Existing Rails v3 users would be safe as they have gem 'validates_hostname', '~> 1', and document they should remain with v1, and everyone else move to gem 'validates_hostname', '~> 2'

@KimNorgaard
Copy link
Owner

@KimNorgaard Since most of the code remains the same, we could write a module with all the common code, and then maintain Rails v3 and Rails v4+ wrapper files and load one or the other as appropriate for the Rails version.

However, considering that the last release of Rails v3 was 10 years ago, I would say it's much simpler to bump to version 2 to indicate a breaking change. Existing Rails v3 users would be safe as they have gem 'validates_hostname', '~> 1', and document they should remain with v1, and everyone else move to gem 'validates_hostname', '~> 2'

I'm fine going forwards with v2.0.0. Please resolve the conflicts, update version info and let me know and I'll restart the review, tag the release and release a new gem.

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.

Calling ActiveRecord::Base.send(:include, PAK::ValidatesHostname) has side effects
2 participants