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

Reverse the included relation name formatting #204

Closed
wants to merge 3 commits into from

Conversation

schtibe
Copy link
Contributor

@schtibe schtibe commented Feb 10, 2016

See #203 for more information

@schtibe
Copy link
Contributor Author

schtibe commented Feb 11, 2016

I have yet to figure out how the unit testing works...

@jerel
Copy link
Member

jerel commented Feb 11, 2016

As far as the tests go python runtests.py should get you up and running. Is there something specific you're wondering about? You can add a function to this file https://github.com/django-json-api/django-rest-framework-json-api/blob/develop/example/tests/integration/test_includes.py and it should just be ran with the tests. Name it similar to the ones in there and if you add single_entry as function parameters pytest will know to create mocks for you.

@schtibe
Copy link
Contributor Author

schtibe commented Feb 16, 2016

I think for it to test the existing code I'd have to rename all the 'comments' and 'authors' to singular.. which has an effect on the "relationships" too.. not sure if it's OK to do that?

edit Ok I found a (hacky) way to at least test the singularize :)

parse_relation = getattr(settings, 'JSON_API_PARSE_INCLUDE_KEYS', None)
singularize_included = getattr(settings, 'JSON_API_SINGULARIZE_INCLUDE_TYPE', None)

settings.JSON_API_PARSE_INCLUDE_KEYS = 'underscore'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pytest-django provides a good way of doing this: http://pytest-django.readthedocs.org/en/latest/helpers.html#settings

@birkholz
Copy link
Contributor

Quote from #257:

It seems that #204 is taking things further by also reversing pluralization and adding a setting to specify which format the keys should be reverted to. In my opinion, the effort of that PR should be expanded to the other areas that perform this behavior, namely parsing. This could then be considered a stopgap until that larger development is ready.

I suggest this PR be expanded to also apply the reversal to attribute/relationship parsing here.

@mblayman
Copy link
Collaborator

mblayman commented Mar 1, 2017

Hi, @schtibe. Thanks for the contribution! This branch is now in conflict in the main branch. Are you interested in fixing this up or should we close out this PR? I'm stepping up to maintain the PR queue and get DRF JA changes flowing again, but I can't merge conflicted stuff.

If this is still valuable, resolving the merge conflicts will give me a chance to review it well. If this is no longer important to you, closing the PR is right next step.

Please let me know what you'd like to do. Thanks!

@schtibe
Copy link
Contributor Author

schtibe commented Mar 1, 2017

Hi @mblayman. I'd like to see if I can fix the the conflict, but I'm currently not even sure anymore if the issue adressed in this PR really complies with the JSON API standard. Please give me some time as I cannot check it right away.

@schtibe
Copy link
Contributor Author

schtibe commented Apr 26, 2017

Ok I don't see the possibility any time soon to investigate further.

@schtibe schtibe closed this Apr 26, 2017
@auvipy
Copy link
Contributor

auvipy commented Nov 13, 2019

will try to investigate this in near future

@auvipy
Copy link
Contributor

auvipy commented Oct 11, 2021

@sliverc do you see the approach of this PR right?

@sliverc
Copy link
Member

sliverc commented Oct 11, 2021

@auvipy The code base has changed quite a bit so it does not make sense to look at a PR. I will add some comments how to address this in still open issue #203

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.

7 participants