openedx / edx-notes-api

edx-notes-api
GNU Affero General Public License v3.0
14 stars 53 forks source link

fix: set JSONRenderer as the DEFAULT_RENDERER_CLASSES #403

Closed CodeWithEmad closed 4 months ago

CodeWithEmad commented 4 months ago

This will introduce a similar approach as edx-platform to set JSONRenderer as the DEFAULT_RENDERER_CLASSES. with this, we don't have Browsable APIs anymore. also, some code reformatting with black and RST cleanup in README.

See here for more context: https://github.com/overhangio/tutor-notes/pull/35

openedx-webhooks commented 4 months ago

Thanks for the pull request, @CodeWithEmad! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

kdmccormick commented 4 months ago

Thanks for the PR @CodeWithEmad . I think the JSON change looks good. Let me see if there is a maintainer that'd want to review this before I merge it.

Regarding black, there is a cost associated with reformatting, in that it complicates git blame and makes it harder for folks with forks to rebase. So, my rule of thumb is that we should only reformat it if we also enforce the new style in CI so that we never need to apply the same reformatting again. Please remove the black commit from this PR, but feel free to introduce it in a new PR along with a black CI check if that interests you.

(the docs reformatting is good to stay -- the considerations about git and forks do not apply so much :)

CodeWithEmad commented 4 months ago

Sure @kdmccormick. Thanks for the explanation. I always make sure to leave any code base cleaner than how I found it. :)

e0d commented 4 months ago

@CodeWithEmad I've run the test. I've also added you to the triage team as you are covered under an entity CLA. This means that tests should run automatically for you in the future. Let me know if that doesn't happen.

CodeWithEmad commented 4 months ago

Thanks @e0d

CodeWithEmad commented 4 months ago

Let me know if that doesn't happen.

I pushed new changes but: "This workflow requires approval from a maintainer." @e0d

mphilbrick211 commented 4 months ago

@CodeWithEmad the tests have been re-run - looks like some checks are still failing.

CodeWithEmad commented 4 months ago

That's super odd. I've added 1 DRF config and the tests are failing on installing the dependencies! I'll look into it.

CodeWithEmad commented 4 months ago

@mphilbrick211 All checks have passed.

openedx-webhooks commented 4 months ago

@CodeWithEmad 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

kdmccormick commented 4 months ago

Thanks @CodeWithEmad !