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

chore: switch to ActiveModel #96

Closed
wants to merge 4 commits into from

Conversation

raszi
Copy link
Contributor

@raszi raszi commented May 7, 2018

This is a breaking change since the previous version required the validations
to be defined separately. And there are changes in the method definitions too.

ActiveModel uses a different approach, it requires validations to be defined per attribute.

With the previous version:

validates :large_number, length: { minimum: 20 }
validates :large_number, numericality: true

In the new version:

validates :large_number, numericality: true, length: { minimum: 20 }

Why is it good?

Why is it bad?

  • a breaking change
  • more methods on the base class

@coveralls
Copy link

coveralls commented May 7, 2018

Coverage Status

Coverage increased (+0.0009%) to 98.575% when pulling a30ef7c on raszi:switch-to-activemodel into 7cfa698 on flexirest:master.

@raszi
Copy link
Contributor Author

raszi commented May 25, 2018

@andyjeffries could you please take a look on this?

@andyjeffries
Copy link
Collaborator

Hi @raszi , sorry for the late reply. I did see this but have been struggling with what to do with it. I don't want to release a breaking change within a minor version bump, but in itself it's not enough for a v2 bump. Maybe if it was a non-default option (e.g. you had to set Flexirest::Base.use_active_model_validators = true) then I'd put it in to a v1 release, but not sure for now how to best handle it.

@raszi
Copy link
Contributor Author

raszi commented May 25, 2018

Well, if it is a breaking change then it requires a major version bump. It does not need to be big even a super small change could be breaking.

Do you agree with my cons and pros? Because if you do then I don't think you should keep the old validations since those are somehow just the duplication of the already existing ActiveModel validations with a slightly different syntax and less extensibility.
Don't worry too much about the version bump, if you'll have more breaking changes then you'll bump it again, you won't run out of them. :)

If you really want to keep your validations or you don't want to introduce a breaking change then I have a different proposal.

Let's create an intermediate class which will include everything as the current Base class but the Validation module of yours. Modify the Base class to extend this new class and include the Validation module.

With this modification if somebody does not want to use the provided validations they could depend on this intermediate class and not on the Base and include the ActiveModel::Validations module.

@andyjeffries
Copy link
Collaborator

I really like that idea, with an update to the docs pointing people in the right direction, it's the best of all worlds.

@raszi
Copy link
Contributor Author

raszi commented May 25, 2018

So you want to go with my second proposal and keep your old validations? Or the original to remove them? 😆

@andyjeffries
Copy link
Collaborator

I liked this idea:

Let's create an intermediate class which will include everything as the current Base class but the Validation module of yours. Modify the Base class to extend this new class and include the Validation module.

Sorry, it wasn't clear ;-)

I can't think of a good name for it though ;-)

@raszi
Copy link
Contributor Author

raszi commented May 28, 2018

Okay, let me figure it out. I'll open a separate PR for that change.

@andyjeffries
Copy link
Collaborator

Nice one, I'll close this PR then.

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.

None yet

3 participants