rubyconfig / config

Easiest way to add multi-environment yaml settings to Rails, Sinatra, Padrino and other Ruby projects.
Other
2.1k stars 230 forks source link

Remove `dry-validation` from dependencies #333

Closed BuonOmo closed 6 months ago

BuonOmo commented 1 year ago
Follow-up of #317, as I'm not a StuartApp member anymore.

> Fixes https://github.com/rubyconfig/config/issues/276 > > # Problem > > Having the dependency on a fix dry-validation version generates multiple issues: > > - users who want another version of the dry-validation gem are blocked > - the dry-validation gem itself has dependencies, making the package size bigger, even if not needed. > > # Solution > > We remove the dependency, but still check *at runtime* that the dependency is met *if and only if* the schema validation feature is used. > > # Other aspects > > All of the changes described below are in a separate commit, I can revert it if it helps merging! > > ## Ruby 2.1 compatibility > > During the implementation, I had a bug with `Config` reloading. The `lib/config/compatibility.rb` introduces a bug that is really hard to debug (for me at least): > > ```ruby > # in config.gemspec > require_relative "config/version" # => true > > # somewhere after > p Config.const_defined?(:CONFIG) # => true > require "config" # => true > # will remove Config and then require everything... Except what was > # already required, e.g. "config/version" > p Config.const_defined?(:CONFIG) # => false > ``` > > Since this file was added for an EOL ruby version (2.1, per commit https://github.com/rubyconfig/config/commit/c4119fb2a31f40759718a0169e2b59a6f86c9aad), I removed it. > > ## Usage of `require_relative` > > Helps be sure that we talk about the same file! Otherwise, it depends on `$LOAD_PATH` (or `$:`) > > ## `Dir[..].sort.each { require }` > > Make sure that the require order is not OS, or filesystem dependent > > > ## `.ruby-version` > > I've removed the file, as for `Gemfile.lock`, the ruby version will in the end depend on the person using the gem, so developers of the gem should also be able to run it with their local ruby versions! > > ## `Config::Error` > > Since there are now two possible errors, lets regroup those under the same banner! (not that it is not a breaking change since `.is_a?` will still behave the same)

@cjlarose to answer your question:

What is the experience if someone specifies a validation_contract instead of using config.schema as described in the README? Is a runtime check still performed to ensure we're using a compatible version?

If think that the issue might be that another version of dry doesn't match this API:

https://github.com/rubyconfig/config/blob/f2b8d2a4b75bca0aca63602232a5ca72b4d156a3/lib/config/validation/validate.rb#L15-L19

Eg the result of calling validator may give an object that doesn't quack #success? nor #errors (and then #text for each error).

However if we want to be that precise, then I guess that we'd also need to check that the validator is actually a dry object.

I have three solutions in mind:

  1. check in Config::Validation::Validate#validate! that if Dry::Validation::VERSION exist in the codebase it is of correct version. At that point it should be either loaded by user, or it will be checked later by the mechanism introduced in this PR
  2. rescue the validate_using! method and print out that there might be a dry-validation version error
  3. skip the validate! method if dry-validation is not found or if it isn't the correct version. Error if Config.schema or Config.validation_contract was set.

I'd go for 3.. It fails early and has a lower footprint on the codebase.

cjlarose commented 1 year ago

I have three solutions in mind:

1. check in `Config::Validation::Validate#validate!` that _if_ `Dry::Validation::VERSION` exist in the codebase it is of correct version. At that point it should be either loaded by user, or it will be checked later by the mechanism introduced in this PR

2. rescue the `validate_using!` method and print out that there might be a dry-validation version error

3. skip the `validate!` method if dry-validation is not found or if it isn't the correct version. Error if `Config.schema` or `Config.validation_contract` was set.

I'd go for 3.. It fails early and has a lower footprint on the codebase.

3 sounds good to me!

BuonOmo commented 1 year ago

Here it is, passing the tests as well.

I don't have much time now, and nor #reload! nor #validate! are tested yet. So I didn't add the test.. If this is blocking for you, I might need up to two extra months due to my personal schedule :/

cjlarose commented 1 year ago

Tests appear to be red on GitHub Actions. It's not immediately clear to me why. Take all the time you need 🙏

pkuczynski commented 1 year ago

@BuonOmo and @cjlarose I fixed the broken CI pipeline...

BuonOmo commented 1 year ago

@cjlarose I took me a while to get my head around this again. But it seems to me that there is no blocker anymore, is there? Also you'd have to reapprove the tests as I had to rebase :/

cjlarose commented 6 months ago

Released with version 5.3.0