-
Notifications
You must be signed in to change notification settings - Fork 965
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
Adding description_content_type to json API #3820
Conversation
@nixjdm thank you for your contribution! The XMLRPC endpoints are planned for deprecation. As such, we generally do not add features to them except to improve compatibility with the previous implementation. You may choose to either split this into two Pull Requests to be considered/discussed individually (one for XMLRPC and one for JSON) or remove the XMLRPC change altogether. |
10-4 @ewdurbin. I have removed the xmlrpc changes from the branch / PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be great to see additional test coverage to ensure that if a content type is set, it is correctly returned. not necessarily a blocker, what do you think @di?
Sorry for the mixed signals about the XML-RPC stuff, @nixjdm. I'll defer to @ewdurbin here. I'm a little surprised to see that the test for this endpoint just has a bunch of |
I was too. I'd be happy to take a crack at expanding the tests to be more
useful for this and other fields in the json api tests. As you said, it
isn't just this field that's testing what is basically the trivial case.
If you make a follow-up ticket, you can assign it to me.
If it's a blocker for this PR, I suppose I should keep the scope narrow,
and expand the tests further for the rest of the json api in a follow-up.
…On Wed, Apr 25, 2018, 7:04 PM Dustin Ingram ***@***.***> wrote:
Sorry for the mixed signals about the XML-RPC stuff, @nixjdm
<https://github.com/nixjdm>. I'll defer to @ewdurbin
<https://github.com/ewdurbin> here.
I'm a little surprised to see that the test for this endpoint just has a
bunch of Nones for the fields... I agree that it'd be nice to see this
value set on the release and assert that the same value is in the result
from the view.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3820 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADRb8jDo8IKliKyVT3BkSABmZmSxeB52ks5tsQ75gaJpZM4TkMS4>
.
|
Yep, I think this is probably ideal. |
…tion_content_type, which is added to the json_api. Adding a separate dedicated test for null values.
843a3dd
to
6473a7a
Compare
@di I updated the tests. A quick explanation of them: I tried to add a non-trivial test case for There was previously 3 releases for to the test project, with the 2nd being the most detailed. I felt adding to the complexity of the second release meant awkward looking code, so I instead made the final release the detailed one. I made the first the empty release. So the order of the releases tested is switched around (in a way I think is more intuitive), but all of the other tests except the one with a How does this look? |
Fixes #3798
I have tested this locally, and extended the existing unit tests for the JSON and XMLRPC APIs. I also extended the API documentation to show these changes.