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

fix: Camelize query parameters #1002

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

dobro322
Copy link
Contributor

I noticed that when using djangorestframework-camel-case, parameters don't convert to camel case.

@tfranzel
Copy link
Owner

@dobro322 thanks for the PR. nice catch, but I think this is too broad. I see that they added a middleware just recently to add that functionality.

So users may have order versions or just not using the middleware, would have the wrong outcome. I think this needs to be a bit smarter about that.

vbabiy/djangorestframework-camel-case@cf76d7a

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (355df90) 98.61% compared to head (a11b545) 98.61%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1002   +/-   ##
=======================================
  Coverage   98.61%   98.61%           
=======================================
  Files          68       68           
  Lines        8281     8286    +5     
=======================================
+ Hits         8166     8171    +5     
  Misses        115      115           
Impacted Files Coverage Δ
...ctacular/contrib/djangorestframework_camel_case.py 100.00% <100.00%> (ø)
...sts/contrib/test_djangorestframework_camel_case.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dobro322
Copy link
Contributor Author

@tfranzel any idea on how to handle it?
Maybe we could add it as a bool config parameter

@tfranzel
Copy link
Owner

tfranzel commented Jun 12, 2023

Maybe we could add it as a bool config parameter

I'd prefer not to, especially since this is a special case for one contrib package only.

any idea on how to handle it?

I would probably retrieve the middleware list from django and check if that middleware is present. Based on that, I would activate the change. This also needs to be safe for older versions of the lib.

@dobro322
Copy link
Contributor Author

This also needs to be safe for older versions of the lib.

Could there be any "unsafe" behavior if we just

from django.conf import settings

def camelize_serializer_fields(result, generator, request, public):
    ...

    camelize_parameters = (
        "djangorestframework_camel_case.middleware.CamelCaseMiddleWare"
        in getattr(settings, "MIDDLEWARE", [])
    )

    ...

    if camelize_parameters:
        #do stuff 

?


the only problem i see is that if someone tries to make a child class from CamelCaseMiddleWare – we wouldn't know it by just checking if string is in array

@tfranzel
Copy link
Owner

Could there be any "unsafe" behavior if we just

unsafe in the sense that when used with older version that there will be no import error or stuff like that.

I believe the middleware list of strings gets automatically converted (imported) on loading. So in theory one could do instanceof.

We are never gonna cover every case anyway. If someone further customizes, they also have to customize the extensions.

@dobro322
Copy link
Contributor Author

Nope, it's just array of strings

image

But i can loop through it and import classes with importlib

@tfranzel tfranzel merged commit a11b545 into tfranzel:master Jun 19, 2023
25 of 32 checks passed
@tfranzel
Copy link
Owner

Thanks, apparently only DRF directly imports their string.

Anyways, I played around a bit and fixed it I think with e98b40d on top of your changes. Thanks @dobro322

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.

None yet

2 participants