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

Aliases for custom method #66

Open
xchi opened this issue Feb 21, 2020 · 14 comments
Open

Aliases for custom method #66

xchi opened this issue Feb 21, 2020 · 14 comments

Comments

@xchi
Copy link

xchi commented Feb 21, 2020

We want to implement a custom method with payload key name as metricValues, but the proper ruby method name should be snake_case.

class MetricPeriodSerializer < Panko::Serializer
        attributes :period, :metricValues

        def metricValues
           [{
            'name' => 'metric1',	
            'value' => object.metric_1	
          },	
           {	
             'name' => 'metric2',	
             'value' => object.metric_2	
           }]
        end

end

We have tried the following way, but having no luck. Just wondering does panko support alias for custom method by any chance?

class MetricPeriodSerializer < Panko::Serializer
        attributes :period, :metricValues
        aliases metric_values: :metricValues

        def metric_values
           [{
            'name' => 'metric1',	
            'value' => object.metric_1	
          },	
           {	
             'name' => 'metric2',	
             'value' => object.metric_2	
           }]
        end

end
@yosiat
Copy link
Owner

yosiat commented Feb 27, 2020

Hi @xchi !

Looks like what you need is support for casing, so you can write something like this:

class MetricPeriodSerializer < Panko::Serializer
  attributes :period, :metric_values, :first_name

  def metric_values
      [{
      'name' => 'metric1',	
      'value' => object.metric_1	
    },	
      {	
        'name' => 'metric2',	
        'value' => object.metric_2	
      }]
  end
end

and the output of the keys will be period, metricValues and firstName.

Now the only question is how we want to support it, currently, Panko don't have global configuration and I want to delay it.

So we can have: specify it on the serializer level like attributes or when serializing. I prefer on the serializer level.

If this makes sense to you, let me know and I'll implement it.

@xchi
Copy link
Author

xchi commented Feb 28, 2020

Hey @yosiat ,

It will be very helpful if you can support casing for our project, as we basically need to create aliases for almost all fields we want to parse.

On the serializer level as one attribute is fine for us as well.
Thanks a lot for your replying.

Vivian

@yosiat
Copy link
Owner

yosiat commented Mar 7, 2020

@xchi let's make sure the api makes sense to you and I'll implement it.

class MetricPeriodSerializer < Panko::Serializer
  keys_format :camel_case

  attributes :period, :metric_values, :first_name

  def metric_values
      [{
      'name' => 'metric1',	
      'value' => object.metric_1	
    },	
      {	
        'name' => 'metric2',	
        'value' => object.metric_2	
      }]
  end
end

We will start by defining the serializer class and maybe in the future we will create a global configuration.
Until then, you can create a base class for serializer (like ApplicationSerializer) which will have only the key_format definition.

@mikebaldry
Copy link
Contributor

In my mind, the simplest and most obvious way to achieve this is to just not use snake_case in your serializers. No other code should be referencing the attribute methods in your serializer, and I think the clearest possible way of doing it, is just using the attribute names as you expect them to come out after serialization.

Also I think global configuration should not be a thing. It's just another name for a global variable that affects the result of something that was pure before.

Using a base serializer should be all that is needed to support that.

Side note @yosiat: I'd like to pick this feature up, if you disagree with my first statement and think it has value.

@yosiat
Copy link
Owner

yosiat commented Mar 9, 2020

@mikebaldry about global configuration, I totally agree with you.

About specifying the names of the attributes as snake case - this can be really problematic for a developer - database column names are not snake case and linters will yell that methods are not snake case.

In addition to that, think about serializer that is used for external API and some specific integration needs to be output as snake case with a subset of fields. In today's case, you need another serializer for this, in ideal place - you will re-use the same serializers and pass options for filters and key format.

I am ok with you taking this feature up (and really happy with that)! but let's make sure we are aligned on the public API for panko and let's get @xchi on board with us.

@mikebaldry
Copy link
Contributor

Sure, I see your points :)

Happy to implement like the example above and we can go from there?

@yosiat
Copy link
Owner

yosiat commented Mar 9, 2020

@mikebaldry cool, let's go for it!

as how I see the implementation, there is no need to change something in the C-extension part (except aliases I think, need to test it). since each attribute have name_sym and name_str , the name_sym used for accessing the data and name_str for the key - see this function - https://github.com/yosiat/panko_serializer/blob/master/ext/panko_serializer/attributes_writer/common.c#L3

Please make sure that your PR includes testing for - attributes (both methods and simple attributes), aliases and associations (both has_one and has_many) and docs.
If you want me to join and help with docs & tests, let me know.

@mikebaldry
Copy link
Contributor

I've had a little first attempt working.. 9d352a4

I have a few things:

  • I just added upcase and downcase as they were simplest to write and test with, probably not any use case for them and I'll remove them when we decide on the below:
  • Adding camelcase would mean:
    • depending on ActiveSupport or something to provide camelize (not ideal)
    • writing our own camelize and friends (not exactly terribly hard, but doesn't seem the best way)
    • "passing through" the transformer - e.g. key_transformer :camelize would just call camelize on String and if it wasn't there, it would cause a NoMethodError.
  • Also supporting calling a block from C code (I haven't researched how to do this yet) like: key_transformer { |key| key[0..5] } or something.
  • key_transformer may not be a great name, I didn't think too hard, I like english though: transform_keys_with :camelize or something seems nicer to me.
  • That commit works for simplest case, but may not work with associations, as I have not written tests for that case.
  • See any problems with my C code? My Ruby C API knowledge is learned as I go (and my C is rusty from 10+ years ago mostly) :)

Many thanks

@xchi
Copy link
Author

xchi commented Mar 12, 2020

Hey @mikebaldry ,

I had look the implementation and it seems can fix our problem. The only question I had is whether you need to provide all support for key_transformer , or this one is going to be an user defined thing?

@mikebaldry
Copy link
Contributor

@xchi I don't know yet. Waiting on @yosiat for guidance :)

@yosiat
Copy link
Owner

yosiat commented Mar 13, 2020

@mikebaldry hi! sorry, another long work-week :)

If you can make a PR and it will be simpler to discuss on the code.

Some notes:

  • I don't think we need to change the C code, the descriptor already contains all attributes and associations and all of them, as far as I remember a name for serialization and name for fetching relevant data - once we serialize, as we do for the filters we should run the key transformer (in Ruby side).
  • I didn't think about key_transformer accepting a Proc, but this is really cool!
  • Camelize - I don't want a dependency on ActiveSupport, it will be too much for just two functions and I think we can use simple gems for achieving inflections - this looks really nice - https://github.com/dry-rb/dry-inflector
  • transform_keys_with sounds like a good choice, let's go with it!

@yosiat
Copy link
Owner

yosiat commented Mar 18, 2020

@mikebaldry hi! any update on this? can I help you?

@oyeanuj
Copy link
Contributor

oyeanuj commented Mar 21, 2020

@yosiat Just adding +1 to the camelize support, that would be super helpful!

@xchi
Copy link
Author

xchi commented Apr 22, 2020

Just want to check any update to this one?

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

No branches or pull requests

4 participants