-
Notifications
You must be signed in to change notification settings - Fork 104
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
Comments
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. |
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 |
@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. |
I'm also happy to work on such a PR. 😇 |
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 |
I think we should allow both for now, with plans to deprecate later at |
How about |
Deprecations
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
My vote is on @njbbaer's suggestion of |
@franzliedke pls feel free to do so.
|
Hello everyone. Can I get some input on the open questions on my PR #393? 🙏🏼 |
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. |
I got the replies I needed and will get back to my PR soon. Currently away from my computer for a few more weeks… |
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.)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.The text was updated successfully, but these errors were encountered: