Skip to content

Support formatting URL segments via new FORMAT_LINKS setting #876

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

Merged
merged 6 commits into from
Dec 27, 2020

Conversation

platinumazure
Copy link
Contributor

Fixes #790

Description of the Change

Added new setting JSON_API_FORMAT_LINKS. This setting is used to inflect serializer properties (i.e., field names) on URLs specifically.

This allows users to have different inflections for properties in JSON responses (using JSON_API_FORMAT_FIELD_NAMES, existing behavior) vs properties in relationship and related resource links (using JSON_API_FORMAT_LINKS, new behavior, previously no inflection was supported).

I used JSON_API_FORMAT_LINKS as the setting name, but I feel it's very important we select a setting name that is as intuitive for users as possible. So, if we think this name could be improved, I'd love to discuss that here and then make a change once we have reached consensus.

This is my first contribution to a Python project, so please rip this apart 😄 Note that I was able to run black, flake8, and pytest individually, but I was not able to get tox to work, so I apologize in advance if I've missed something as a result.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@codecov
Copy link

codecov bot commented Dec 24, 2020

Codecov Report

Merging #876 (8a2656d) into master (3833271) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #876      +/-   ##
==========================================
+ Coverage   97.74%   97.80%   +0.06%     
==========================================
  Files          60       60              
  Lines        3319     3320       +1     
==========================================
+ Hits         3244     3247       +3     
+ Misses         75       73       -2     
Impacted Files Coverage Δ
rest_framework_json_api/settings.py 100.00% <ø> (+8.00%) ⬆️
rest_framework_json_api/relations.py 88.46% <100.00%> (+0.05%) ⬆️
tests/test_relations.py 100.00% <100.00%> (ø)
tests/test_views.py 100.00% <100.00%> (ø)
example/tests/test_views.py
example/tests/test_relations.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3833271...8a2656d. Read the comment docs.

Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. Great work! There are some small comments I've added otherwise the PR is shaping up nicely.

Concerning naming of FORMAT_LINKS: I see that this could be misleading. But I can only come up with names which are really long and as the documentation states it well how it is used I think it is OK for me to leave it with this name (unless we come up with something better)

@platinumazure
Copy link
Contributor Author

platinumazure commented Dec 27, 2020

Thanks @sliverc for the review!

I've addressed most of your feedback. The only outstanding item at this point is how to "inline" the urlconf in tests/urls.py; the URLs must be set up in order for the RelatedMixin links to reverse-resolve properly. So I need to figure out how to move them but still be able to use @pytest.mark.urls, or else I need guidance on mocking the reverse-resolution functionality to avoid depending on a urlconf.

If you have any suggestions off the top of your head, I would be grateful to hear them. Otherwise, I'll try to look into it some more in the next week or so.

Regarding setting names, what do you think of JSON_API_FORMAT_RELATED_FIELD_LINKS? It doesn't do a great job of indicating what URL segments are being formatted, but it does make it clear that the related and relationship links are the only links formatted, and those only appear for related fields.

@platinumazure
Copy link
Contributor Author

I've made the requested changes, and I believe this is ready for another review. Thanks!

Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@sliverc sliverc merged commit c503748 into django-json-api:master Dec 27, 2020
@platinumazure
Copy link
Contributor Author

platinumazure commented Dec 27, 2020

Oh darn it, I forgot to change the name to what I had suggested in my other comment.

@sliverc Thoughts on JSON_API_FORMAT_RELATED_FIELD_LINKS or JSON_API_FORMAT_RELATED_LINKS? Slightly more specific but not too wordy IMO.

I can create a new PR if you're okay with changing the setting name to one of the two I suggested, just let me know.

@platinumazure platinumazure deleted the related-url-formatting branch December 27, 2020 19:24
@sliverc
Copy link
Member

sliverc commented Dec 28, 2020

I think in the future if there are other parts of the urls being automatically determined by DJA the setting should be called FORMAT_LINKS. Currently this PR only affects related links so renaming it to FORMAT_RELATED_LINKS is a good idea. If in the future if more parts of the various links are generated automatically we can always deprecate FORMAT_RELATED_LINKS and move towards using FORMAT_LINKS.

It would be great if you could create a PR. Thanks.

@platinumazure
Copy link
Contributor Author

Will do, I'll have another PR up in the next day or two.

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.

inflection ignored in lookup by RelationshipView
2 participants