-
Notifications
You must be signed in to change notification settings - Fork 23
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
Use Dry::Struct instead of Virtus #59
Conversation
So there was a significant difference in behavior between 0.2 and 0.3 of dry-struct. Further, the master branch doesn't work at all (some bug with features they're adding regarding inheritance.) So I have some concern about the stability of that project - but it DOES have a maintainer. This only took a day to do, and there were backwards compatible steps in the middle. Thoughts on if we should actually go through with this? |
One thing I want to note - I do like the Dry::Types coercion a lot better than the ones in Virtus, and we can share them with Reform. |
Still digging into the code a bit, but I found a fairly concise comparison between dry-struct and Virtus at http://dry-rb.org/gems/dry-struct/
|
Yup, that's a good summary. This PR relates as such:
This PR adds writers.
👍 This PR uses those.
Yes - in 0.2 the "schema" constructor worked exactly the way I wanted. In 0.3 I had to overwrite the
👍 This seems fine, but it just so happens what's most useful to us is the
Right now this is pretty useless to us - I'm doing everything I can to avoid nested objects. |
I should mention that I had a version of the writers which were immutable - you'd get a new instance of |
The issues I ran into between versions are documented here: dry-rb/dry-types#198 |
3ade4e2
to
b0e07f3
Compare
I was able to get rid of the |
@@ -2,13 +2,13 @@ | |||
module Valkyrie::Persistence::Fedora | |||
class DynamicKlass |
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.
Not in scope for this PR, but I'd prefer DynamicClass
here -- only using klass
or clazz
where it would conflict.
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.
#61 created.
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.
Looks good. A couple of stylistic/semantic comments, but no objection to merging.
app/types/valkyrie/types.rb
Outdated
@@ -4,6 +4,12 @@ module Types | |||
include Dry::Types.module | |||
ID = Dry::Types::Definition | |||
.new(Valkyrie::ID) | |||
.constructor { |input| ::Valkyrie::ID.new(input) } | |||
.constructor do |input| | |||
Valkyrie::ID.new(input) |
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 indentation tripped me.
lib/valkyrie/active_model.rb
Outdated
base.include Virtus.model | ||
base.include Draper::Decoratable | ||
base.extend ClassMethods | ||
class Model < Dry::Struct |
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.
Filename / classname mismatch. Consider model.rb
?
b0e07f3
to
0e1b9b6
Compare
This isn't a totally faithful conversion - effectively it implements writers where Dry::Struct doesn't want writers. There's some other weirdness in here around the fact that the newer versions of Dry::Struct won't cast parameters that you don't pass in the initializer.