Skip to content

Feature: Member Name Styles #12

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

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

Conversation

vovimayhem
Copy link

@vovimayhem vovimayhem commented Feb 8, 2018

What does this PR do?

Addresses #11 by implementing the following changes:

  • Adds a key option member_name_style, which configures the style used for member names:
    • dasherize/hyphen (using dashes)
    • titleize/humanize (using spaces)
    • underscore (default, using underscores)
  • Adds a method member_name_style, which configures the above-mentioned styles on all members.

@richmolj
Copy link
Contributor

richmolj commented Feb 8, 2018

Thanks for pitching in @vovimayhem! We should absolutely be supporting different key formatting.

Wanted to get your thoughts on an alternate tact here. The two things I'm looking for:

  • key(:"a-hyphenated-key") seems friendlier and more obvious
  • Avoiding the activesupport dependency.

I think we can get this without too much trouble. Under the hood, we essentially say json[key] == record.send(key). record.send('a-hyphenated-key') obviously fails, which is the error you see in the Issue you created.

Instead, what about allowing users to set a "default comparison" proc instead of relying on that default? This way we'd avoid the activesupport dependency, support camelCase and any other custom comparison logic a dev might need. Something like:

JsonapiSpecHelpers::Payload.register(:person) do
  self.default_comparison = ->(key, record) { record.send(key.underscore) }
end

The issue here is we would probably need a way to globally set this as well as override. Another option would be to set our default proc to use respond_to?(:underscore) - this would avoid the activesupport dependency and work for camelCase, kebab-case as well as under_scored.

Thoughts?

@vovimayhem
Copy link
Author

vovimayhem commented Feb 9, 2018

@richmolj :

  • I'd prefer key :some_key over key :'some-key' or key 'some-key'. Using "string symbols" like :'this-one' or :'This One' has never been obvious to me - AFAIK, it's not a common practice at all.
  • By keeping it like key :some_key is easier to infer the record's method/attribute your'e after. If it's not the method name your'e after, you can always fall back to the proc.
  • The member_name_style config is implemented either as a "per-key" or "per-payload" configuration.
  • I would rather use a self.some_config = :some_value over using a proc for a "per-payload" configuration... Having a proc for that is a chunk of code that repeats in every payload. We can always implement String#dasherize if it's not available without depending on activesupport.
  • I like the idea to allow users alternately use a proc, in case they need it.
  • For the config name, I prefer member_name_style over default_comparison, as it relates to the jsonapi spec rather than the actual functionality or implementation... but other suggestions are welcome.

@vovimayhem
Copy link
Author

An alternate idea (although I'm going as far as change the current DSL) to use a 'test/assertion' language for the payload DSL:

JsonapiSpecHelpers::Payload.register :person do
  expect_attribute 'first-name'
  expect_attribute 'nickname', allow_nil: true
  expect_relationship 'current-company'
end

This way, the user is expected to put the exact attribute he/she is expecting to find in the payload. JsonapiSpecHelpers will only have to assume that it's going to use record.send(key.underscore) if no proc is given for an expected attribute.

@richmolj
Copy link
Contributor

richmolj commented Feb 9, 2018

I think these are good ideas for maybe a future release but I'd like to keep backwards-compatibility for now. Seems like the simplest thing we could do to solve the issue without any developer-facing changes would be changing this line to:

prc ||= lambda do |record|
  # alternatively, copy the method from activesupport and add to this repo
  name = name.underscore if name.respond_to?(:underscore)
  record.send(name)
end

Would that work for you?

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.

2 participants