Skip to content
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

String/Lambda support for conditional attributes/associations #1699

Merged
merged 1 commit into from
Apr 26, 2016

Conversation

mtsmfm
Copy link
Contributor

@mtsmfm mtsmfm commented Apr 21, 2016

Purpose

I want to exclude attribute if its association/attribute is nil.

:if option accepts only symbol at this time so we must write condition like following:

class UserSerializer < ApplicationSerializer
  has_one :game, if: :game_exists?

  def game_exists?
    object.game
  end
end

But it is not bad to support String like rails:

class UserSerializer < ApplicationSerializer
  has_one :game, if: 'object.game'
end

I don't have any use case for lambda(proc) at this time but many developers who experienced rails will guess that it accepts lambda.

Related GitHub issues

#1403

@beauby
Copy link
Contributor

beauby commented Apr 21, 2016

Yeah, I had support for lambda in mind when adding this feature, so 👍 on the idea of this PR.

def condition_lambda
case condition
when Symbol
-> (serializer) { serializer.public_send(condition) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure building a throwaway lambda here is the best way. How about the following

def condition_lambda(serializer)
  case condition
  when Symbol
    serializer.public_send(condition)
  when String
    serializer.instance_eval(condition)
  when Proc
    condition.call(serializer)
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it can be a method 👍
Fixed and force pushed 😄

@beauby
Copy link
Contributor

beauby commented Apr 21, 2016

LGTM – I'll merge unless somebody raises concerns.
Would you mind adding corresponding docs for the feature?

@mtsmfm mtsmfm force-pushed the str-lambda-support-for-if branch from bf5cd79 to c1fd222 Compare April 21, 2016 15:01
when Proc
condition.call(serializer)
else
fail ArgumentError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to include a message here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

By the way, it is better if raise an error on evaluate serializer's DSL I think.

https://github.com/rails-api/active_model_serializers/pull/1699/files#diff-689b80d81fb899042cc36668903c9a3dR10

@mtsmfm mtsmfm force-pushed the str-lambda-support-for-if branch 3 times, most recently from 9e43a67 to 72b7191 Compare April 21, 2016 16:26
@mtsmfm
Copy link
Contributor Author

mtsmfm commented Apr 22, 2016

Hmm, tests for jruby-9.0.4.0 on appveyor are failed but passed on travis 😕

@beauby
Copy link
Contributor

beauby commented Apr 24, 2016

@mtsmfm Looking into the appveyor thing.

@mtsmfm mtsmfm force-pushed the str-lambda-support-for-if branch from 72b7191 to a50cd44 Compare April 24, 2016 12:31
@mtsmfm
Copy link
Contributor Author

mtsmfm commented Apr 24, 2016

@beauby I noticed appveyor setting for JRuby is wrong and fixed (b33791b
)

@mtsmfm mtsmfm force-pushed the str-lambda-support-for-if branch from a555028 to b33791b Compare April 24, 2016 16:04
@beauby
Copy link
Contributor

beauby commented Apr 24, 2016

@mtsmfm Awesome! Would you mind submitting the AppVeyor settings fix as a separate PR?

@@ -3,21 +3,19 @@ version: '{build}'
skip_tags: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebased!

@mtsmfm mtsmfm force-pushed the str-lambda-support-for-if branch from b33791b to aa087a2 Compare April 26, 2016 12:37
@bf4 bf4 merged commit 0433869 into rails-api:master Apr 26, 2016
@mtsmfm mtsmfm deleted the str-lambda-support-for-if branch April 27, 2016 00:33
@nicolas-besnard
Copy link

nicolas-besnard commented May 4, 2016

This is great for one field, but what if I've a lot a field I wanted to hide ? Do I have to add a predicate after each attribute / association ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants