method-inc / bamboozled

Bamboozled wraps the BambooHR API without the use of Rails dependencies.
MIT License
44 stars 50 forks source link

`client.employee.all` and `client.meta.all` return different key types (Symbol vs String) #3

Closed lepht closed 9 years ago

lepht commented 9 years ago

I ran into this issue when converting some code from using client.meta.all to client.employee.all. From the example code in the repo:

employee_data = client.meta.users.map do |e|
    next unless e[:employeeId]
    client.employee.find(e[:employeeId], %w(displayName department hireDate terminationDate))
 end.compact.reject{|e| e['hireDate'] == '0000-00-00'}

IMO: It would be nice for both methods to be consistent, so that the above code could be switched to using client.employee.all without needing to switch all fields from symbols to strings, eg. e[:employeeId] becomes e['employeeId']

lepht commented 9 years ago

I also just noticed an inconsistency between the two methods and the name of the id field: employeeId vs id

markrickert commented 9 years ago

Thanks for the idea... I'm wondering if there'd be a way to implement HashWithIndifferentAccess without including ActiveSupport?

Maybe make it a dependency with require: false then require 'active_support/core_ext/hash/indifferent_access'?

Thoughts, @Enriikke?

markrickert commented 9 years ago

Fixed in version 0.0.6. Thanks for reporting this!

lepht commented 9 years ago

@markrickert thanks for the quick fix!

Re: HashWithIndifferentAccess behavior without requiring ActiveSupport... I happened across this SO question and one answer recommended your require method, but there's also an answer that uses default_proc to get the same behavior but it's limited to read operations.

This is semi ugly, IMO, and I'm wondering if it doesn't just make sense to avoid both methods and do a quick transformation on your JSON.parse before returning the hash, eg:

Hash[json.map{ |k,v| [k.to_s, v] }]
# or 
Hash[json.map{ |k,v| [k.to_sym, v] }]

The downside being that you'd break existing code that was relying on the old behavior -- so maybe your way is better ;)