-
Notifications
You must be signed in to change notification settings - Fork 37
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
Refactor constraints #156
Refactor constraints #156
Conversation
Realized I did not update the documentation, but would like to get feedback on this so moving to draft |
This was also my only concern here. It also takes away from what might be other use cases for validation. For example, we might want to allow similar things to what activerecord validation provides, such as :format or :comparison. That is I could see something like: opt :max_volume, :type => :integer
opt :start_volume, :type => :integer
validates :start_volume, comparison: { less_than_or_equal_to: :max_volume } So I think the way forward is give a more clear name for the minmax, but that will likely hold up the refactoring. I'd be ok with a separate PR with just the refactorting, since that's much clearer and I'm basically ready to merge now, and then the mixmax bit can be a smaller PR. I'm still not sure of a realistic use case is for mix max, which makes it a little harder for me to understand. |
9ce021f
to
b5e5d7d
Compare
Reverted this to a plain old refactoring of the depend/conflict/either constraints. Can tackle min/max validation in another PR. |
I'm surprised that no specs failed with the changes in wording. Are these tested? |
No, apparently not. The difference in the algorithm for determining success/failure of the Constraints would make it impractical to match the old failure messages. Should I put together some tests for the failure messages before closing out this PR? |
Yes, please - even a few basic tests would be sufficient. |
b5ad3b3
to
80d2980
Compare
DependConstraint
,ConflictConstraint
,EitherConstraint
) with an abstractConstraint
class.Addvalidate
method which points to newMinMaxConstraint
, plus tests.tbh i'm not too sure about thevalidate
name, the name feels a little unclear about its purpose to me.@miq-bot add-label refactoring
@miq-bot add-reviewer @Fryguy