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

Adding description_content_type to json API #3820

Merged
merged 5 commits into from
Apr 26, 2018
Merged

Conversation

nixjdm
Copy link
Contributor

@nixjdm nixjdm commented Apr 25, 2018

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.

@ewdurbin
Copy link
Member

@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.

@nixjdm
Copy link
Contributor Author

nixjdm commented Apr 25, 2018

10-4 @ewdurbin. I have removed the xmlrpc changes from the branch / PR.

Copy link
Member

@ewdurbin ewdurbin left a 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?

@ewdurbin ewdurbin changed the title Adding description_content_type to json and xmlrpc APIs Adding description_content_type to json API Apr 26, 2018
@di
Copy link
Member

di commented Apr 26, 2018

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 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.

@nixjdm
Copy link
Contributor Author

nixjdm commented Apr 26, 2018 via email

@di
Copy link
Member

di commented Apr 26, 2018

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.

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.
@nixjdm
Copy link
Contributor Author

nixjdm commented Apr 26, 2018

@di I updated the tests. A quick explanation of them: I tried to add a non-trivial test case for description_content_type, leave most everything else untouched, and prep the other values for easily modifying their test values later for the test_detail_renders.

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 description_content_type are otherwise unchanged. I also kept testing of an info section with mainly empty field values. To accomplish that I made it a separate dedicated test. The diff here in github looks a bit weird in regards to the url sections of the two defs. The second def is entirely new, and so the first urls section should be the one that looks partially changed.

How does this look?

@di di merged commit 76f1acc into pypi:master Apr 26, 2018
@nixjdm nixjdm deleted the api-content-type branch April 26, 2018 21:52
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

4 participants