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

Simplify jsonapi_class configuration #92

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JoeWoodward
Copy link

@JoeWoodward JoeWoodward commented Sep 1, 2018

Adds

• JSONAPI::Rails::SerializableClassMapping class
Overriding Hash’s lookup can be confusing without creating an
descendent class.

  • Old behavior
    inferrer.class == Hash
    Doesn’t make it obvious that there’s custom behavior
  • New behavior
    inferrer.class == JSONAPI::Rails::SerializableClassMapping
    Now it’s obvious where to look for the unusual behavior
    This setup also allows us to define the default mappings and the
    lookup behavior in separate configuration options
    • configuration options for
  1. jsonapi_class_mapper
  2. jsonapi_class_mappings
  3. jsonapi_errors_class_mapper
    (fallback to jsonapi_class_mapper if nil)
  4. jsonapi_errors_class_mappings

Removes

• configration options for

  1. jsonapi_class
  2. jsonapi_errors_class

@JoeWoodward JoeWoodward force-pushed the feature/default_serializer_pairs branch 3 times, most recently from d605448 to b05a5e5 Compare September 1, 2018 15:50
Adds
• JSONAPI::Rails::SerializableClassMapping class
  Overriding Hash’s lookup can be confusing without creating an
  descendent class.
  - Old behavior
    inferrer.class == Hash
    Doesn’t make it obvious that there’s custom behavior
  - New behavior
    inferrer.class == JSONAPI::Rails::SerializableClassMapping
    Now it’s obvious where to look for the unusual behavior
  This setup also allows us to define the default mappings and the
  lookup behavior in separate configuration options
• configuration options for
  1. jsonapi_class_mapper
  2. jsonapi_class_mappings
  3. jsonapi_errors_class_mapper
    (fallback to jsonapi_class_mapper if nil)
  4. jsonapi_errors_class_mappings
Removes
• configration options for
  1. jsonapi_class
  2. jsonapi_errors_class
@JoeWoodward JoeWoodward force-pushed the feature/default_serializer_pairs branch from b05a5e5 to 975793f Compare September 1, 2018 16:07
@JoeWoodward
Copy link
Author

Looks like this PR would benefit this issue too #68

@JoeWoodward
Copy link
Author

Just added tests against multiple versions of Ruby and Rails. Updates what you were doing here #46 @beauby

@JoeWoodward JoeWoodward force-pushed the feature/default_serializer_pairs branch 7 times, most recently from a01e8f0 to 8b885e0 Compare September 2, 2018 08:16
@JoeWoodward JoeWoodward force-pushed the feature/default_serializer_pairs branch from 8b885e0 to 0d1cddf Compare September 2, 2018 09:52
@caselas
Copy link

caselas commented Mar 21, 2019

This looks good to me, and solves some confusion I'm seeing on my project. Any chance we can move it forward?

@JoeWoodward
Copy link
Author

@dawidof are you maintaining this now? Any verdict on whether contributions to this repo are welcome? Seems like @beauby is either busy or has lost interest.

@dawidof
Copy link
Member

dawidof commented Apr 1, 2019

hi @JoeWoodward. Yeah, I'm maintaining, but at the moment I have some health problems, I'll have more time in few weeks. According to your PR:

  1. Make rebase with master
  2. Here are some breaking changes https://github.com/jsonapi-rb/jsonapi-rails/pull/92/files#diff-fe0bb8be91e2254f2ea1d2e48474bfceR4. It would be good to have them in changelogs

Contributions are welcomed

@beauby
Copy link
Member

beauby commented Apr 5, 2019

@JoeWoodward Yes, I haven't had time to give the love this project deserves, and @dawidof is in charge now.

This PR does several things (update TravisCI configuration, jsonapi payload for invalid request, and change of the lookup mechanism) – would you mind splitting it up into semantic chunks?

Regarding splitting the lookup in two parts, one for static mapping, and one for dynamic mapping, I do not see the value, as it makes the code more complex. The current behavior, where the inferrer is a Hash (which means it can have default values that are cached), does not seem confusing to me – could you elaborate?

@JoeWoodward
Copy link
Author

Hey, @dawidof, @beauby! Thanks for the responses, been so long I almost forgot I wrote these PRs

Just had to read my code again for a while to remember the issues I was experiencing. The reason I made a custom class that inherited from Hash to do the mapping was due to wasting a lot of time trying to figure out how the mappings worked when I was build a project last year...
When debugging the issue I had, I was using pry to inspect jsonapi-rails at runtime. I think I was trying to pass a class to it that was not being found or something, can't remember, but the important piece that I do remember is banging my head against the wall thinking.

inferrer
=> {}
inferrer.class
=> Hash
# all normal

inferrer[:String]
# I would expect this to return nil but returns the serializer
=> SerializableString

inferrer
=> { :String => SerializableString }
# Even as a senior ruby developer, this really stumped me. 

It's not often that you see Hash objects that do not follow the standard behavior of a Hash so it's easy to forget that it's even possible to modify the lookup logic, trying to debug that was really confusing. I believe it took me over a day to find the config that showed the Hash documentation; by changing the class name you make it really obvious where too look. Now when you call inferrer.class you know exactly where to look. It's actually still a Hash so shouldn't change the cachability of it, although I did just notice that I didn't cache it and dup, I create a new one each time, not sure if that really changes anything performance wise though.

Regarding splitting the dynamic/static values up and modifying the Hash lookup behavior in the initialize method instead. That may be more code and more complex for maintainers but it also simplifies the configuration for the developers that use the gem. I know juniors in my company were really struggling with some of this stuff and changing this made it easier for them to understand. Now they don't have to decipher what Hash.new { |h, k| ... } does; They just have to worry about how to mutate the class name into the serializer classes now.

Thanks for the feedback, I will try to find some time to split this PR up.

@JoeWoodward
Copy link
Author

Also, sorry to hear about your health problems @dawidof, hope you're recover{ed,ing} now.

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

Successfully merging this pull request may close these issues.

4 participants