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

Remove CamelCasing relationship names #440

Closed
dominikb opened this issue Mar 3, 2020 · 5 comments
Closed

Remove CamelCasing relationship names #440

dominikb opened this issue Mar 3, 2020 · 5 comments
Labels
enhancement New feature or request next major We'll revisit this feature in the next major version

Comments

@dominikb
Copy link
Contributor

dominikb commented Mar 3, 2020

This is a follow-up ticket for issue #393. (and #366, #332, #307)

Although I personally use camelCased relationship names, I don't think this is a behavior we want to enforce. Especially because there have been multiple tickets regarding this feature.

The conversion happens in two places. Namely in AddsFieldsToQuery and in AllowedInclude.

I see multiple ways to handle removing the camel casing:

  1. Removing the feature with a notice in the changelog.
  2. Remove the feature, detect if a cast would be necessary and throw a meaningful exception.
  3. Keep the camel casing and dynamically detect if a conversion is required.

What are your thoughts?

CC: @suth

@suth
Copy link

suth commented Mar 3, 2020

Although it's documented the behavior surprised me even the second time I ran into it. I don't think there's a problem with choosing one default over the other, but if an alias is specified I think it could be safely assumed that casting is not necessary.

Is there any impact on nested relationships as well? Haven't had to use that feature yet.

@dominikb
Copy link
Contributor Author

dominikb commented Mar 4, 2020

Nested relationship names will be cast as well. (Can be seen in this test case)

I just found an older todo that refers to validating that included relationships do exist:

// TODO: Check for non-existing relationships?

Could we remove camel casing and add this check to give the user a meaningful exception?

@pionl
Copy link

pionl commented Mar 13, 2020

Hi guys, I would recommend to be able to use a config entry to determine if CamelCase should be used?

@AlexVanderbist
Copy link
Member

I agree that this is not the best default behaviour. Initially, I only added this because I didn't like the look of case sensitive URL parameters 🙈. Is there any reason to keep this in? I'd accept a PR that just gets rid of this on the v3 branch.

I'd accept a PR for that non-existing relationship exception as well.

@AlexVanderbist AlexVanderbist added enhancement New feature or request next major We'll revisit this feature in the next major version labels Mar 29, 2020
@freekmurze
Copy link
Member

We'll take a look at this in the next major version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next major We'll revisit this feature in the next major version
Projects
None yet
Development

No branches or pull requests

5 participants