-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
Limit max version of dry-validation #223
Conversation
OK, I'm pretty sure I can't make code climate happy since it thinks a line is too long, but if I put it on multiple it tells me it wants it on one again. In lieu of any rewrites, can we please get a quick win on limiting the version? |
@@ -17,8 +17,9 @@ | |||
# Require `belongs_to` associations by default. Previous versions had false. | |||
Rails.application.config.active_record.belongs_to_required_by_default = true | |||
|
|||
# Removed in rails 5.2+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed only for tests for Rails 5.0. There is a separate 5.2 folder. So this change should not be required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It blows up on rake spec
. That must run the wrong combination of directories and versions? I had trouble getting appraisal to work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, made some changes. Lets see if build fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally happy if it's not needed.
lib/config/version.rb
Outdated
@@ -1,3 +1,3 @@ | |||
module Config | |||
VERSION = '1.7.1' | |||
VERSION = '1.7.2'.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not bump the version. freeze
is OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured it needed to be bumped so bundler recognizes a new version when it's pushed. I'm used to doing it myself for work, forgot that might be part of a different process elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will upgrade this whenever the gem will get released. Saying that, I can release once this PR is merged I guess.
config.gemspec
Outdated
if RUBY_VERSION >= '2.1' && RUBY_VERSION < '2.2' | ||
s.add_dependency 'dry-validation', '~> 0.10', '>= 0.10.7', '< 1.0.0' | ||
end | ||
s.add_dependency 'dry-validation', '~> 0.10', '>= 0.10.7', '< 1.0.0' if RUBY_VERSION >= '2.1' && RUBY_VERSION < '2.2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code climate complained that this line was too long. This is how I had it originally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not seem to complain anymore?
Hmm it now fails in many places due sqlite issue. Would you have some time to debug? |
Sure, but it will probably be in the morning. |
Appraisals
Outdated
@@ -1,34 +1,42 @@ | |||
appraise 'rails-3' do | |||
gem 'sqlite3', "< 1.4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use single quotes instead of double please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops
We are getting very close! Still few things failing I am afraid :( |
@pkuczynski Why are there always new code climate failures? |
OMGeee. I got the tests to pass. @pkuczynski Can we tell code climate to ignore those string macros or do you need me to add them? |
Code climate fixed :) |
@pkuczynski all good? |
Looks good to me! :) Should I release it? You also put a lot of effort into this PR - would you like to become a contributor? |
Please release it! |
Already released :) |
How about the contribution part? |
Sure. That would be great. |
Done! :) Welcome onboard :) |
Version 1.0 is a breaking change:
https://github.com/dry-rb/dry-validation/blob/master/CHANGELOG.md#v100-2019-06-10
Fixes #222