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

Updated to dry-schema 1.0 #224

Merged
merged 6 commits into from
Jun 20, 2019

Conversation

rdubya
Copy link
Contributor

@rdubya rdubya commented Jun 18, 2019

This is an attempt at supporting dry-validations/dry-schema ~>1.0. I took the approach of raising the minimum required ruby version to do this, but I can probably make it backwards compatible if you feel that is needed.

The main reason for this is because we use this gem in our application and we are trying to use some new gems that depend on the dry-* gems but this is forcing us to use an older version of those gems.

@pkuczynski
Copy link
Member

Thanks for all that work! I think it would be good to keep it backward compatible, so other people can still use it. There might be a lot of old apps which depend on this gem...

Would that be a lot of work for you?

@rdubya
Copy link
Contributor Author

rdubya commented Jun 19, 2019

I think I can do it, the main issue is probably how to define the dependencies. With these changes the dependency is actually on the dry-schema gem instead of the dry-validation gem since they separated the two in 1.0. I think I could leave the dependency on dry-validation since it has a dependency on dry-schema so will make sure it gets loaded, even though we don't need any functionality from the new dry-validation gem.
How should I define the dependency now? It seems like the only way we could allow both to be used is by having the dependency set to dry-validation, '>=0.12.2' and then put instructions in the readme to lock the version to something less than 1.0 in their gemfile if they are using something lower than ruby 2.4, or does bundler check the supported ruby version and only use the latest version that works with their version of ruby?

Would you consider putting it out as a 2.0 release so people can stick to 1.x if they need to and jump to 2.0 if their ruby version supports it?

.travis.yml Outdated
# Rails 4.2.1 recommends Ruby 2.2 and requires 1.9.3 or newer
# Rails 5 requires Ruby 2.2.2 or newer
# Sinatra 2 requires Ruby 2.2.2 or newer
# Rails 4 and 4.1 fails with Ruby newer then 2.3
- gemfiles/rails_3.gemfile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to go too?

Appraisals Outdated
gem 'sqlite3', '< 1.4.0'
gem 'rails', '4.0.13'
end
# These appraisals fail on anything newer than 2.3 and the gem now only supports 2.4+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove those, please...

config.gemspec Outdated

s.add_dependency 'activesupport', '>= 3.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we drop Rails 3.x we should upgrade this to >= 4.2.0?

@pkuczynski
Copy link
Member

After thinking about it for a while I believe you right. We should bump version of this gem to 2.0 and require ruby >= 2.4. That would mean dropping support for rails < 4.2. Would you agree? We should also remove Rails 3 I guess?

You would need to update Changelog, Readme and delete unused test rails apps from spec/app/

@pkuczynski
Copy link
Member

Thanks for all your hard work so far! :)

@pkuczynski
Copy link
Member

We would need to close, merge and release this gem before Friday, as later on, I would be unavailable for the next 2 weeks... Do you have some time to work on this, or should I take over?

@pkuczynski
Copy link
Member

Seems that you put quite a lot of effort into this PR. Would you like to become a contributor?

@rubyconfig rubyconfig deleted a comment Jun 19, 2019
@rubyconfig rubyconfig deleted a comment Jun 19, 2019
@rdubya
Copy link
Contributor Author

rdubya commented Jun 19, 2019

Hey @pkuczynski, I think I can get this wrapped up tonight/tomorrow night. Most of the changes you commented on should be fairly quick. We can drop support for Rails < 5 if you want. The specs all run fine for rails 3 with newer versions of ruby, but 4.0 and 4.1 don't work. Though, 4.2 is EOL so there probably doesn't need to be support for it anymore. It's probably fine to leave it at 3 for now to continue to provide it for a wider range of people as long as it isn't taking extra work to support it. I 'll go whichever way you think is best.

Sure I'll be a contributor, thanks!

@pkuczynski
Copy link
Member

I think Rails 3 is not supposed to work on Ruby 2.4 and having it in, while not having Rails 4 feels odd, don't you think?

I think we should keep Rails 4.2 as the minimum supported version :)

Don't forget to fix codeclimate/codacy issues.

@pkuczynski
Copy link
Member

Just invited you for becoming a maintainer. Welcome on board ;)

@rubyconfig rubyconfig deleted a comment Jun 20, 2019
@rubyconfig rubyconfig deleted a comment Jun 20, 2019
@rubyconfig rubyconfig deleted a comment Jun 20, 2019
@rdubya
Copy link
Contributor Author

rdubya commented Jun 20, 2019

@pkuczynski Thanks for the invite. I think I have everything cleaned up that you mentioned. Let me know if you see anything else that you think should be changed.

@rubyconfig rubyconfig deleted a comment Jun 20, 2019
@pkuczynski
Copy link
Member

Looks good! Thanks!

@pkuczynski pkuczynski merged commit c7896dd into rubyconfig:master Jun 20, 2019
@pkuczynski
Copy link
Member

@rdubya should I release 2.0 now or wait for #227?

@rdubya rdubya deleted the upgrade-dry-validations branch June 20, 2019 12:15
@pkuczynski pkuczynski added this to the 0.3.pre milestone Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants