Replace Virtus with dry-types #1920

dnesteryuk · 2019-10-18T07:25:20Z

As we know Virtus isn't maintained anymore, so it makes sense to use something different for coercing. There is a dry-types lib which was created as a replacement for Virtus (at least a part of it). Although, it is only about coercion.

This change gets rid of Virtus and adds dry-types to the stack. Almost, all tests pass. However, there are a few tests which were modified, actually, they didn't make any sense. Yeah, Virtus was providing
some "magic" to make them green, but I think they were unreal.

The structure inside didn't change match. I tried to integrate dry-types without huge refactoring.

If people want to use Virtus for custom types, it is possible after implementing a parse method which initializes a model.

class User
  include Virtus.model

  attribute :id, Integer
  attribute :name, String

  def self.parse(attrs)
    new(attrs)
  end
end

This PR is kind of "work in progress", I would like to gather feedback then proceed. If somebody can add this change to their project and launch tests, it will be awesome, the goal is to avoid breaking changes.

Missing parts:

It closes #1347.

myxoh · 2019-10-18T15:08:19Z

Following this, shout if you get stuck or need some help

dblock · 2019-10-20T16:36:19Z

👀

dblock · 2019-10-22T12:47:46Z

Gemfile

@@ -9,6 +9,7 @@ group :development, :test do
  gem 'hashie'
  gem 'rake'
  gem 'rubocop', '0.51.0'
+  gem 'virtus'


This can't be used at all in the final PR.

There are a few tests which use Virtus to verify that it can be used as a custom type, it means I need to remove the 🤔

I worry that we'll leave something that works in tests but not in prod. Maybe those can become integration tests with their own Gemfile?

You are right, Virtus isn't needed anymore. I removed it and adjusted tests testing custom types 🙂

dblock · 2019-10-22T12:48:41Z

UPGRADING.md

@@ -1,6 +1,39 @@
 Upgrading Grape
 ===============

+### Upgrading to >= 1.2.5
+
+[Virtus](https://github.com/solnic/virtus) isn't used for coercing anymore. If your project depends on Virtus, you have to add it to your `Gemfile`. Also, if Virtus is used as a custom type


Simlify and shorten, "has been replaced by dry-types for parameter coercion" and just ", add it to your Gemfile"

dblock · 2019-10-22T12:50:01Z

UPGRADING.md

+  end
+end
+```
+


I think we want to explain how to rewrite a custom type with dry-types here, first. If I am upgrading I'd rather not use Virtus unless I can't.

Dry-types cannot be used in place of Virtus for custom types, dry-schema might be. So, if users need something like Virtus they need to keep it or migrate to dry-schema. I guess it is up to them to decide, the goal is to avoid breaking changes, so if they use Virtus, they can still use it until they have time to migrate.

It sounds like you're saying that if you want a custom type you have to use Virtus? Or are you saying you can use Virtus? Either way let's make this very clear in the upgrading/readme doc.

No, Virtus isn't required for custom types. As it was before, any class implementing the parse method might be used. 🙂 I've added an additional sentence to the upgrading doc.

dblock · 2019-10-22T12:51:38Z

lib/grape/validations/types/array_coercer.rb

+
+        # This method maintaine logic which was defined by Virtus for arrays.
+        # Virtus doesn't allow nil in arrays.
+        def would_virtus_reject?(val)


I know it's Virtus related, but I would just call this reject?. It's about the current coercer, not some library-specific behavior.

dblock

This LGTM once it's cleaned up & al. Nice work.

dblock · 2019-10-22T12:52:35Z

lib/grape/validations/types/custom_type_coercer.rb

                end
+              rescue StandardError => _e


This is too broad of a catch-all, I think should be a parser error only.

We don't know which error might be raised by a custom type. Users define it. 🙂

That's why it's a problem. Catch all could be something you actually don't expect and there's no way to know if you catch all. We should not be catching all here at all or having an explicit contract with a coercer. If you want the latter, introduce an exception such as CoercionError and rescue that. Document that coercers should throw this or a child of this exception.

You are right, this begin/rescue isn't required. Most likely I added to fix some broken tests, after refactoring, it became useless.

dblock · 2019-10-22T12:53:05Z

lib/grape/validations/types/dry_type_coercer.rb

+require 'dry-types'
+
+module DryTypes
+  include Dry.Types()


Remove (), or maybe just use Dry.Types.... everywhere. It's liberally used below as either.

I would love to do that, but it is how dry-types should be used. After calling Dry.Types() registered types get added to a container, it involves adding constants and some other meta-programming.

Ok, I think I am missing something. I'm probably just bad at Ruby :) I am staring at dry-types types.rb code and I don't get it. How does it make Dry.Types different from Dry.Types()?

Dry.Types is a method which returns an instance of a module to be included. 🙂 Here is that module.

dnesteryuk · 2019-10-31T07:37:20Z

Dry-types requires Ruby 2.4 or greater 😞

dblock · 2019-10-31T11:26:46Z

Dry-types requires Ruby 2.4 or greater 😞

Deprecate Ruby 2.3 with a note, changelog, readme change, etc.

dnesteryuk · 2019-11-01T07:53:31Z

Hi @dblock I made recommended changes, please, have a look again. ☺️ Thanks for your patience! 😄

dblock

I am good with merging this via a squash with some minor changes in docs, see above.

Since it's a big one, @myxoh, care to take another look?

dblock · 2019-11-01T17:55:35Z

README.md

@@ -167,6 +167,8 @@ The current stable release is [1.2.4](https://github.com/ruby-grape/grape/blob/v

 ## Installation

+After **1.2.5**, Ruby 2.4.0 or newer is required.


This should go into UPGRADING, for this doc just state that Ruby 2.4 or newer is required.

dblock · 2019-11-01T17:55:50Z

UPGRADING.md

+end
+```
+
+you need to add a class-level `parse` method to the model:


Start with a cap, "Add a ..."

dblock · 2019-11-01T17:56:09Z

UPGRADING.md

+
+#### Ruby
+
+After adding dry-types, Ruby 2.4.0 or newer is required.


Let's put this section on top of the list?

dblock · 2019-11-01T17:56:23Z

UPGRADING.md

+end
+```
+
+Custom types which don't depend on Virtus don't require any changes.


Add a "For more information see ..." and link to this PR.

dnesteryuk · 2019-11-06T06:57:16Z

@dblock I've made changes to README and UPGRADING. Please, have a look.

dblock · 2019-11-06T13:10:07Z

I'm good with this! @myxoh want to confirm and merge?

dblock · 2019-12-01T16:33:16Z

Let's get this build to green? There's some legit looking failure with rack-edge. If it's rack's fault we can skip or mark pending and open an issue. I also released 1.2.5 so this will need an update in CHANGELOG and a rebase.

dblock · 2019-12-01T16:33:55Z

UPGRADING.md

@@ -1,6 +1,49 @@
 Upgrading Grape
 ===============

+### Upgrading to >= 1.2.5


Let's make this 1.3.0. Also change in README/version.rb.

dnesteryuk · 2019-12-02T08:34:30Z

@dblock that commit broke Grape tests 😞

As we know Virtus isn't maintained anymore, so it makes sense to use something different for coercing. There is a dry-types lib which was created as a replacement for Virtus (at least a part of it). Although, it is only about coercion. This change gets rid of Virtus and adds dry-types to the stack. The structure inside didn't change match. Dry-types was integrated without huge refactoring. If people want to use Virtus for custom types, it is possible after implementing a parse method which initializes a model. class User include Virtus.model attribute :id, Integer attribute :name, String def self.parse(*args) new(*args) end end

dnesteryuk · 2019-12-02T10:02:48Z

spec/grape/api_spec.rb

@@ -1133,7 +1133,7 @@ class DummyFormatClass

      subject.use Rack::Chunked
      subject.get('/stream') { stream test_stream }
-      get '/stream', {}, 'HTTP_VERSION' => 'HTTP/1.1'
+      get '/stream', {}, 'HTTP_VERSION' => 'HTTP/1.1', 'SERVER_PROTOCOL' => 'HTTP/1.1'


@dblock Here is how I fixed the problem with Rack

That's totally fine, we just want to make sure Grape is compatible.

dblock · 2019-12-02T14:52:35Z

I merged this, thank you so much for doing this work @dnesteryuk, it's a big one. I suggest you email the mailing list describing this change and asking people to give the master branch a try, especially if they do custom type checking.

flvrone · 2019-12-02T15:02:14Z

@dblock I've just tried to run a test suite on one of my projects against your master branch, and all green 😁 There's no custom type checking though.

dnesteryuk · 2019-12-04T07:33:10Z

@dblock where is the mailing list? 😬

dblock · 2019-12-04T11:01:39Z

@dblock where is the mailing list? 😬

http://groups.google.com/group/ruby-grape

nbulaj · 2019-12-26T08:25:28Z

Any reason why we tied to version 1.1.x of dry-types? @dnesteryuk I know gem changes even with patch version and can bring some incompatibilities, but ... I don't think it's a good solution btw

I tried Grape from the master branch with Ruby 2.7 on my current project and see a lot of deprecation messages like:

/.../ruby-2.7.0/gems/dry-types-1.1.1/lib/dry/types/any.rb:18: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call

Even if it's already fixed or would be fixed in the future versions of Dry-types we can't update to it due to version restrictions :(

dblock · 2019-12-26T11:33:11Z

Any reason why we tied to version 1.1.x of dry-types? @dnesteryuk I know it changes even with patch version changes, but ... I don't think it's a good solution btw

We should relax it, please PR.

It was removed in Grape 1.3: ruby-grape/grape#1920 / https://github.com/ruby-grape/grape/blob/master/UPGRADING.md#upgrading-to--130 I'm not sure this particular constant definition is useful anyway.

dnesteryuk mentioned this pull request Oct 18, 2019

Switch from virtus to dry-types #1347

Closed

dblock reviewed Oct 22, 2019

View reviewed changes

dblock requested changes Oct 22, 2019

View reviewed changes

dnesteryuk changed the title ~~[WIP] replace Virtus with dry-types~~ Replace Virtus with dry-types Oct 29, 2019

dblock requested changes Nov 1, 2019

View reviewed changes

dblock reviewed Dec 1, 2019

View reviewed changes

dnesteryuk commented Dec 2, 2019

View reviewed changes

dblock merged commit 69ce0df into ruby-grape:master Dec 2, 2019

dnesteryuk deleted the chore/bye-virstus-hello-dry-types branch December 4, 2019 07:32

nbulaj mentioned this pull request Dec 26, 2019

Relax dry-types dependency version #1948

Merged

ghiculescu mentioned this pull request May 22, 2020

Remove all reference of Virtus sorbet/sorbet-typed#249

Merged

		@@ -167,6 +167,8 @@ The current stable release is [1.2.4](https://github.com/ruby-grape/grape/blob/v

		## Installation

		After 1.2.5, Ruby 2.4.0 or newer is required.


		#### Ruby

		After adding dry-types, Ruby 2.4.0 or newer is required.

Replace Virtus with dry-types #1920

Replace Virtus with dry-types #1920

Conversation

dnesteryuk commented Oct 18, 2019 • edited Loading

myxoh commented Oct 18, 2019

dblock commented Oct 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnesteryuk commented Oct 31, 2019

dblock commented Oct 31, 2019

dnesteryuk commented Nov 1, 2019

dblock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnesteryuk commented Nov 6, 2019

dblock commented Nov 6, 2019

dblock commented Dec 1, 2019 • edited Loading

dblock Dec 1, 2019 • edited Loading

Choose a reason for hiding this comment

dnesteryuk commented Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Dec 2, 2019

flvrone commented Dec 2, 2019

dnesteryuk commented Dec 4, 2019

dblock commented Dec 4, 2019

nbulaj commented Dec 26, 2019 • edited Loading

dblock commented Dec 26, 2019

dnesteryuk commented Oct 18, 2019 •

edited

Loading

dblock Oct 22, 2019 •

edited

Loading

dblock Oct 23, 2019 •

edited

Loading

dblock left a comment •

edited

Loading

dblock commented Dec 1, 2019 •

edited

Loading

dblock Dec 1, 2019 •

edited

Loading

dnesteryuk commented Dec 2, 2019 •

edited

Loading

nbulaj commented Dec 26, 2019 •

edited

Loading