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

Make field naming more consistent #281

Open
jasonkarns opened this issue Sep 21, 2021 · 12 comments
Open

Make field naming more consistent #281

jasonkarns opened this issue Sep 21, 2021 · 12 comments

Comments

@jasonkarns
Copy link

Fields that are simply "renamed" use: field :internal_name, name: :output_name

But fields that are defined "inline" use field(:output_name) { ... }

In the former case, the argument to field is not the final output name, but in the latter case it is. I find this inconsistency a bit jarring. I understand why it is this way, but it feels backwards to me.

What would be the appetite for an API that did something like: (method name adopted from graphql-ruby's parameter for the same behavior: essentially aliasing.)

field :out_name, method: :internal_name

This way the parameter to field is consistently the resulting key, regardless if the field's key name remains unchanged; if it is renamed, or if it is defined directly in the blueprint.

Copy link

github-actions bot commented Nov 1, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@franzliedke
Copy link

I came here to suggest exactly this.

When I look at the serializer, I want to be able to quickly see the keys of the resulting JSON object at a glance, without having to look at options.

I am happy to open a PR with this feature, but would like to know whether you would accept this idea first.

The API suggested by @jasonkarns would allow for a backward-compatible change: using the old name: keyword would still work, but be marked deprecated, and the method: keyword (or something else, maybe more generic to not prescribe the method of extraction).

@ritikesh
Copy link
Collaborator

@jasonkarns this is a good suggestion - would you be willing to raise a PR for it? Let's ensure backward compatibility is maintained though with a deprecation notice for the older approach.

@franzliedke
Copy link

I'm also happy to work on such a PR. 😇

@jhollinger
Copy link
Contributor

I agree that this proposal makes more sense than what we have today. I know @ritikesh suggested deprecating the old way, but that implies eventually removing the old way, and that could be a significant challenge for large code bases.

To avoid that, what does everyone think about supporting both ways? (Which to give credit where credit's due, was @njbbaer's idea iirc).

field :foop, name: :foop  # a field called "foo" that pulls from a "foop" method
field :bar, method: :barp # a field called "bar" that pulls from a "barp" method

Granted it's a bit confusing using them right beside each other, but that's on whoever writes the code. Presumably an entire app, or at least an entire view or Blueprint, would choose one way and stick with it.

Also, I don't love using method since there's a method instance method on Object. Can anyone think of a better name?

@njbbaer
Copy link
Contributor

njbbaer commented Jan 29, 2024

I think we should allow both for now, with plans to deprecate later at v2.0 and provide a migration script for larger codebases. Is anyone familiar enough with those types of scripts in Ruby to create that or gauge level of effort?

@njbbaer
Copy link
Contributor

njbbaer commented Jan 30, 2024

How about accessor, attribute, or source as an alternative to method?

@franzliedke
Copy link

Deprecations

I know @ritikesh suggested deprecating the old way, but that implies eventually removing the old way, and that could be a significant challenge for large code bases.
To avoid that, what does everyone think about supporting both ways? (Which to give credit where credit's due, was @njbbaer's idea iirc).

Given that 1.0 was released just a few weeks ago, I think it's absolutely legitimate to announce a deprecation now and remove the old variant in 2.0 whenever that gets shipped one day. Especially considering that the deprecated instances are relatively easy to find (or even automate as you suggested).

Supporting both just invites confusion, especially assuming the deprecated behavior wouldn't be documented anymore.

Naming

  • method: not ideal, as sending a message / calling a method is just one possible way of extraction - it wouldn't make sense for hashes
  • accessor: same problem IMO, it hints at a message being called

My vote is on @njbbaer's suggestion of source.

@ritikesh
Copy link
Collaborator

ritikesh commented Feb 5, 2024

@franzliedke pls feel free to do so.

I'm also happy to work on such a PR. 😇

@franzliedke
Copy link

Hello everyone.

Can I get some input on the open questions on my PR #393? 🙏🏼
I would love to finalize this.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@franzliedke
Copy link

I got the replies I needed and will get back to my PR soon. Currently away from my computer for a few more weeks…

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

Successfully merging a pull request may close this issue.

5 participants