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

Replace rest_framework_swagger with drf_yasg #176

Closed
wants to merge 2 commits into from

Conversation

NeOneSoft
Copy link
Contributor

This PR was created following edx/upgrades#17 to switch django rest swagger to drf-yasg

@openedx-webhooks
Copy link

Thanks for the pull request, @NeOneSoft! I've created OSPR-6130 to keep track of it in JIRA, where we prioritize reviews. 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@natabene
Copy link

@NeOneSoft Thank you for your contribution. Please let me know once this is ready for our review.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Oct 13, 2021
@NeOneSoft NeOneSoft changed the title chore: switch to drf-yasg Replace rest_framework_swagger with drf_yasg Oct 13, 2021
@awais786
Copy link
Contributor

@NeOneSoft Please resolve these conflicts.

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #176 (833a2eb) into master (b2e1d5c) will not change coverage.
The diff coverage is n/a.

❗ Current head 833a2eb differs from pull request most recent head 79f1e0e. Consider uploading reports for the commit 79f1e0e to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master      #176   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          412       412           
  Branches        47        47           
=========================================
  Hits           412       412           
Flag Coverage Δ
unittests 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 b2e1d5c...79f1e0e. Read the comment docs.

@NeOneSoft
Copy link
Contributor Author

NeOneSoft commented Dec 15, 2021

Hey there @awais786 !!.I already fix the conflicts. Could you please review this PR.?Thank you so much

@natabene natabene added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 21, 2022
@awais786
Copy link
Contributor

I have verified its working. Please rebase.

Screen Shot 2022-01-24 at 12 57 35 PM

@UsamaSadiq
Copy link
Member

@NeOneSoft The PR is ready to merge now. Could you rebase and resolve the conflicts once so I'll go ahead and merge this PR.
Thanks.

@NeOneSoft
Copy link
Contributor Author

Hi @UsamaSadiq !! I already rebased, resolved the conflicts and squash the commits to clean up the PR, now is ready to check it if you are aggree. Thanks in advance !!

@codecov
Copy link

codecov bot commented Apr 22, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@b2e1d5c). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             master      #176   +/-   ##
==========================================
  Coverage          ?   100.00%           
==========================================
  Files             ?        12           
  Lines             ?       412           
  Branches          ?        64           
==========================================
  Hits              ?       412           
  Misses            ?         0           
  Partials          ?         0           
Flag Coverage Δ
unittests 100.00% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 b2e1d5c...0da05ce. Read the comment docs.

@UsamaSadiq
Copy link
Member

Hi @NeOneSoft thanks for the effort. Seems like the quality failure will also need to be fixed before the PR can be merged. Could you have a look at the failures once you get opportunity. Thanks.

feat: replace rest_framework_swagger with drf_yasg

feat: resolve conflicted dependencies

feat: resolve doc.txt file conflicted dependencies

feat: resolve doc.txt file conflicted dependencies_02

feat: resolve doc.txt file conflicted dependencies_03

feat: resolve doc.txt file conflicted dependencies_04

feat: resolve conflicted files to master

feat: resolve conflicted files to master branch

feat: solve quality tests and conflicted files

feat: solve conflicted files
@natabene
Copy link

@NeOneSoft Did you have a chance to implement the feedback?

@NeOneSoft
Copy link
Contributor Author

Hi@natabene, yes I already modified according the feedback but it seems that I'm having an import issue. I will check it and let you know once it's done. Thanks

@natabene
Copy link

@NeOneSoft Just checking if you had a chance to look into this.

@NeOneSoft
Copy link
Contributor Author

Hi @natabene, sorry for my delay to answer. I will working on it tomorrow and let you know once is ready

@timmc-edx
Copy link
Contributor

Addressed in #226

@timmc-edx timmc-edx closed this Aug 3, 2022
@openedx-webhooks
Copy link

@NeOneSoft Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants