-
Notifications
You must be signed in to change notification settings - Fork 21
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
Introduce an abstract serializer class #61
Conversation
# Conflicts: # lib/surrealist/instance_methods.rb
@AlessandroMinali haven't I missed something? |
I have added possibility to pass context inside serializer constructor: for objects that are not coupled with serializable model, but still have to be accessed from the inside (I guess it will be class IncomeSerializer < Surrealist::Serializer
json_schema { { amount: Integer } }
def amount
current_user.guest? ? 100000000 : object.amount
end
def current_user
context[:current_user]
end
end
class Income
include Surrealist
surrealize_with IncomeSerializer
attr_reader :amount
def initialize(amount)
@amount = amount
end
end
income = Income.new(200)
IncomeSerializer.new(income, current_user: GuestUser.new).surrealize
# => '{ "amount": 100000000 }'
IncomeSerializer.new(income, current_user: User.find(3)).surrealize
# => '{ "amount": 200 }' Also I will write good docs for all of this, but a bit after the feature itself. |
Looks good. Context passing seems a little weird (but functional), but the only alternatives I can think of is using method missing or a branch condition on every method call to check a hash of valid functions which will just be slow. Maybe this could be another DSL (at the cost of adding more complexity for a low use case, maybe not worth)?
|
spec/serializer_spec.rb
Outdated
end | ||
|
||
|
||
|
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.
Extra blank line detected.
spec/serializer_spec.rb
Outdated
json_schema { { name: String } } | ||
end | ||
|
||
|
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.
Extra blank line detected.
@AlessandroMinali I think this DSL might be useful, but we should give it a thought. For now I suggest to merge this PR, if there are no problems or suggestions, and to file an issue concerning the DSL you proposed where we could cover all pros and cons. |
#59