dry-rb / dry-schema

Coercion and validation for data structures
https://dry-rb.org/gems/dry-schema
MIT License
415 stars 108 forks source link

Replace case-in pattern-matching with normal case-when statements #442

Closed r7kamura closed 1 year ago

r7kamura commented 1 year ago

Fix https://github.com/dry-rb/dry-schema/issues/441

r7kamura commented 1 year ago

I made a simple change from case-in to case-when for now, but RuboCop will probably be upset with this code about its Metrics, so I'll have to add some more refactoring.

$ bundle exec rubocop lib/dry/schema/types_merger.rb
Inspecting 1 file
C

Offenses:

lib/dry/schema/types_merger.rb:70:9: C: Metrics/PerceivedComplexity: Perceived complexity for merge_unwrapped_types is too high. [10/8]
        def merge_unwrapped_types(unwrapped_old, unwrapped_new) ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

Tip: Based on detected gems, the following RuboCop extension libraries might be helpful:
  * rubocop-rake (https://rubygems.org/gems/rubocop-rake)
  * rubocop-rspec (https://rubygems.org/gems/rubocop-rspec)

You can opt out of this message by adding the following to your config (see https://docs.rubocop.org/rubocop/extensions.html#extension-suggestions for more options):
  AllCops:
    SuggestExtensions: false
flash-gordon commented 1 year ago

Please replace it with conditional class_eval for ruby 2.7/3+. We'll drop support for 2.7 soon anyway so it'll be easier to go with the PM version later

r7kamura commented 1 year ago

Fair enough. I changed it to use conditional branching by RUBY_VERSION. After the change, I also confirmed that the warning does not appear in Ruby 2.7 as shown below.

$ ruby --version
ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-linux]
$ bundle exec ruby -I lib -e "require 'dry/schema/types_merger'"
solnic commented 1 year ago

Thanks for tackling this. It'd be cleaner to just have two modules and include them conditionally though 😬

flash-gordon commented 1 year ago

@solnic we'll drop it in a month or so, I wouldn't bother :)