Simplify jsonapi_class configuration

JoeWoodward commented 6 years ago

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
5. jsonapi_class
6. jsonapi_errors_class

JoeWoodward commented 6 years ago

Looks like this PR would benefit this issue too https://github.com/jsonapi-rb/jsonapi-rails/issues/68

JoeWoodward commented 6 years ago

Just added tests against multiple versions of Ruby and Rails. Updates what you were doing here https://github.com/jsonapi-rb/jsonapi-rails/pull/46 @beauby

caselas commented 5 years ago

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

JoeWoodward commented 5 years ago

@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 commented 5 years ago

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:

Make rebase with master
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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

jsonapi-rb / jsonapi-rails

Simplify jsonapi_class configuration #92

Adds

Removes